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 Metadata #542

Closed
wants to merge 153 commits into from
Closed

Ephys Metadata #542

wants to merge 153 commits into from

Conversation

garrettmflynn
Copy link
Member

@garrettmflynn garrettmflynn commented Dec 18, 2023

This PR re-enables Ephys metadata, integrating the changes to Ophys metadata (#505) and the last several months of updates.

To run this, make sure your environment is using the correct NeuroConv dev branch (catalystneuro/neuroconv#447), which exposes the data_type property that corresponds to the expected return type of the table properties. Otherwise, all table properties will default to string types.

garrettmflynn and others added 30 commits November 9, 2023 07:52
@garrettmflynn
Copy link
Member Author

@rly Just FYI tested this today and it looks like there's a blocker to convert the Ecephys metadata.

In addition, the relevant NeuroConv branch doesn't support OpenEphys yet

Base automatically changed from ophys-metadata to main January 27, 2024 18:17
@CodyCBakerPhD
Copy link
Collaborator

So, IDK. The more I think about this the more I think it might be best to just do a custom GUIDE-side operation on each recording interface similar to how the ophys metadata breaks up different planes. NeuroConv has a low-code method of attaching properties and doesn't really think of them as tables until they are in the NWB file, so trying to redesign that approach is going to be fairly difficult (especially with schematic validation since this isn't counted as 'metadata')

Probably best to do on a separate/fresh PR that runs on main of NeuroConv, but what I would do is generate a separate HandsOnTable for every recording interface that has columns for each property key (and rows for every channel) of the attached recording_extractor for each interface (there will always be exactly one extractor per interface, but could be multiple interfaces per converter)

@CodyCBakerPhD
Copy link
Collaborator

As far as the table validation goes, that would then give us total control over the structure to enforce on the table, largely inferred from the dtype of the numpy array of that property (which they all should be I believe), which should make it easier to customize here

@garrettmflynn
Copy link
Member Author

@CodyCBakerPhD Would you mind reiterating the conclusions we reached in today's meeting so I can push forward with a new PR confidently?

@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn separate hands on table for each recording interface; then a static non-interactive table that combines all of them to represent the way the actual NWB electrodes table will appear

Although, I think the neurosift preview after the metadata page should effectively handle that part; if that's OK with @rly then we can just have the individual tables exposed on the file metadata page and defer the 'unification' to the NeuroConv write tools + neurosift rendering on that small output

@CodyCBakerPhD
Copy link
Collaborator

The tables will also need to support copy/pasting and the classic 'drag to copy/paste a repeated value' to be really usable if they don't do that already

@garrettmflynn garrettmflynn mentioned this pull request Feb 5, 2024
@CodyCBakerPhD
Copy link
Collaborator

replaced by #586

@CodyCBakerPhD CodyCBakerPhD deleted the ephys-metadata branch March 13, 2024 03:33
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