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

create Electrodes class to replace electrodes table #1205

Closed
wants to merge 4 commits into from
Closed

Conversation

rly
Copy link
Contributor

@rly rly commented Mar 10, 2020

@ajtritt I am trying to create an Electrodes class to use for the electrodes table instead of a DynamicTable to solve #1204. Without being its own class, we cannot specify in PyNWB that certain columns of the electrodes table are optional.

This involves changing the ObjectMapper for NWBFile such that it constructs an Electrodes object instead of a DynamicTable object for the NWBFile.electrodes constructor argument.

I thought of a few ways to do it:

  1. Construct an Electrodes object from the /general/extracellular_ephys/electrodes/ group builder by setting each value and creating each VectorData individually, but this would be very tedious and involve a lot of hard coding.
  2. Construct an Electrodes object from the /general/extracellular_ephys/electrodes/ group builder by setting the values of the Electrodes object to the values of the DynamicTable that is already generated. This doesn't work because I cannot change the parent of the VectorData classes to the new object. That is what I have coded up in the PR here.
  3. Override the container class associated with the /general/extracellular_ephys/electrodes/ group builder. This doesn't seem possible.

Can you think of a better way to do this? Would this even be possible? Would it require substantial changes to HDMF to work?

Stepping back at the source problem, it might be easier to change how adding columns works in DynamicTable to handle optional columns. What do you think?

@rly
Copy link
Contributor Author

rly commented Mar 26, 2020

I thought of another (slightly hacky) way to get around this issue. We can just promote the DynamicTable object constructed by the ObjectMapper to be an Electrodes object and add the new columns to the class. I updated the PR to reflect this method. This requires hdmf-dev/hdmf#323

@rly rly requested review from oruebel and ajtritt March 26, 2020 00:10
@rly rly marked this pull request as ready for review March 26, 2020 00:10
@rly
Copy link
Contributor Author

rly commented Jul 25, 2024

@mavaylon1 is restarting this effort with a new ElectrodesTable neurodata type in #1890. Will close this PR.

@rly rly closed this Jul 25, 2024
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.

1 participant