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

Ephys #2

Merged
merged 18 commits into from
Sep 6, 2024
Merged

Ephys #2

merged 18 commits into from
Sep 6, 2024

Conversation

pauladkisson
Copy link
Member

This PR adds an OpenEphysRecordingInterface and PhySortingInterface tested on a single example session.

Copy link

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

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

Everything here looks good to me.

As a comment that might be useful:

Lately I have started working using a converter pipe instead of the NWBConverter object. I think that some of the things that you do here like manipulating the properties of the recording object are easier to write and understand with that workflow. Plus, if something is wrong you know earlier. Example:

https://github.com/catalystneuro/dicarlo-lab-to-nwb/blob/39656bcd239a405362596c9693eb03cc3ccf8f04/src/dicarlo_lab_to_nwb/conversion/convert_session.py#L66-L74

@pauladkisson
Copy link
Member Author

pauladkisson commented Sep 6, 2024

Lately I have started working using a converter pipe instead of the NWBConverter object. I think that some of the things that you do here like manipulating the properties of the recording object are easier to write and understand with that workflow. Plus, if something is wrong you know earlier.

Interesting, I didn't even know this kind of alternative workflow was possible. Before, if I was making too many changes to an interface I would usually just define a child Interface, but using a ConverterPipe could be a good alternative for some cases.

I think in this case, I'll just stick with the NWBConverter, at least for now, but thank you for the heads up.

As a side note, if we want to normalize using ConverterPipes instead of NWBConverters (in some cases ofc), we should really have some documentation on them. I couldn't find anything about them on the docs

@h-mayorquin
Copy link

h-mayorquin commented Sep 6, 2024

I think in this case, I'll just stick with the NWBConverter, at least for now, but thank you for the heads up.

I think this is wise, probably too much friction to change in the middle of an ongoing project.

@pauladkisson pauladkisson merged commit bf34d65 into main Sep 6, 2024
@pauladkisson pauladkisson deleted the ephys branch September 6, 2024 22:14
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