Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[When min PyNWB >=v2.1.0]: remove manual fillers for optional ElectrodeTable cols #6

Closed
2 tasks done
CodyCBakerPhD opened this issue Jul 7, 2022 · 3 comments
Closed
2 tasks done

Comments

@CodyCBakerPhD
Copy link
Member

What would you like to see added to nwb-conversion-tools?

With PyNWB v2.1.0, various electrode columns (x,y,z, location, imp) are no longer required so we could remove those corresponding lines of our write_recording tools using coded defaults to just not propagate them at all if the recording object doesn't have the information set, and let PyNWB handle all the setting automatically.

What we need to decide (likely best at next group meeting) is if we want to enforce the minimal version of the conversion tools to use most recent of PyNWB, et al.

Is your feature request related to a problem?

No response

What solution would you like?

Remove https://github.com/catalystneuro/nwb-conversion-tools/blob/main/src/nwb_conversion_tools/tools/spikeinterface/spikeinterface.py#L368-L380 and dependent lines.

Do you have any interest in helping implement the feature?

Yes.

Code of Conduct

@h-mayorquin
Copy link
Collaborator

I vote for the minimal version being the latest version.

There is also this code over there that maybe should go:

https://github.com/catalystneuro/nwb-conversion-tools/blob/5969e2074dfd16acd8640f76fe88c6c147edc8a4/src/nwb_conversion_tools/tools/spikeinterface/spikeinterface.py#L269-L274

@CodyCBakerPhD
Copy link
Member Author

I vote for the minimal version being the latest version.

Yeah, probably the best approach. Just waiting for Ryan to debug that thing with it before we can change it here: NeurodataWithoutBorders/pynwb#1497

@CodyCBakerPhD CodyCBakerPhD transferred this issue from catalystneuro/nwb-conversion-tools Jul 21, 2022
@CodyCBakerPhD
Copy link
Member Author

Fixed by #219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants