Skip to content

Conversation

@vigji
Copy link
Contributor

@vigji vigji commented Nov 15, 2024

PR addressing this issue: SpikeInterface/spikeinterface#3539.

Very minimal fix, simply filtering the XML nodes corresponding to enabled probes.

Not sure if this is to be tested? I assume there would be the need for custom asset data, have not looked deeply into it.

@alejoe91
Copy link
Member

Thanks @vigji

If you have a settings file with disabled probes it would be great if you added it to the test/data/openephys and also make small test for it :)

@alejoe91
Copy link
Member

Note that tests are failing becuase probably the isEnabled has been added with v0.7 of the OE NPIX plugin. Can you fix?

@vigji
Copy link
Contributor Author

vigji commented Nov 15, 2024

Indeed, I just checked data from the old setup and indeed it is a 0.7 new feature!

yess, I’ll fix this and add a test.

@vigji
Copy link
Contributor Author

vigji commented Nov 15, 2024

Ok, there should now be new assets and tests for multiple conditions of enabled/disable probes. Let me know how this looks!

@codecov
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.42%. Comparing base (1fc2d35) to head (705c0d1).
Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
src/probeinterface/io.py 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
+ Coverage   89.38%   89.42%   +0.03%     
==========================================
  Files          10       10              
  Lines        1885     1891       +6     
==========================================
+ Hits         1685     1691       +6     
  Misses        200      200              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alejoe91
Copy link
Member

Can you run pre-commit locally? The automatic pre-commit fails because it cannot push to organizations

@vigji
Copy link
Contributor Author

vigji commented Nov 16, 2024

Should be good to go now!

@vigji
Copy link
Contributor Author

vigji commented Nov 16, 2024

Wait before merging, I am having an issue.

I suspected there was something wrong as I expected this behavior to start happening with new version 0.7 of oephys plugin. However, reading the xml file I was finding

<SETTINGS>
  <INFO>
    <VERSION>0.6.7</VERSION>
   ...

So I assumed I was incorrect and that was the version. However, testing the PR on some files recorded with the non-updated system in the lab, I get same version tag and no isEnabled field.

So, it seems that OEphys is writing xml files with the wrong version in them? is this possible?

Definitively to be checked before merging this and causing troubles to anyone recording with 0.6.7!

@alejoe91
Copy link
Member

You could also simply check if the isEnabled field is present and use it if it is. This should be ok for all versions

@vigji
Copy link
Contributor Author

vigji commented Nov 16, 2024

That's what I am doing now locally but I liked the idea of keeping the logic related explicitly to versions. If that is ok I'll push that - although I still want to understand what is happening with the xml files, I opened an issue for that.

Copy link
Member

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vigji see my last comment

@vigji
Copy link
Contributor Author

vigji commented Nov 18, 2024

I actually fixed the issue with version check, which as I mentioned I find more explicit and nicer, if you agree I'll leave it this way!

@vigji vigji requested a review from alejoe91 November 18, 2024 13:35
Co-authored-by: Alessio Buccino <[email protected]>
@alejoe91
Copy link
Member

Thanks @vigji for this great addition! Merging :)

@alejoe91 alejoe91 merged commit 26a7d33 into SpikeInterface:main Dec 2, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants