-
Notifications
You must be signed in to change notification settings - Fork 22
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
Preserve order of recording extractor custom properties in add_electrodes()
#793
base: main
Are you sure you want to change the base?
Conversation
add_electrodes()
add_electrodes()
"rel_x", | ||
"rel_y", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are rel_x
and rel_y
after the custum ones though? I would have expected them before since they are pre-defined in NWB schema
How does this interact with two recording properties that have different columns? And if you write them a in a different order? Is the order still intuitive? The order I would have expected to see on the electrodes table is (i) pre-defined NWB columns (group, location, x, y, z, if present, etc) then (ii) ones added automatically to a format by SI, then (iii) custom ones set after recording interface initialization Also while I think the approach is fine, making use of the implicit ordering property of insertion of keys into a |
How does this interact with two recording properties that have different columns? And if you write them a in a different order? Is the order still intuitive? This is a good point and I didn't think about in for my use case, I would say in this case it could be based on precedence: self.recording_1.set_property(key="custom_property_x", values=[0.1] * self.num_channels)
self.recording_1.set_property(key="custom_property_y", values=[0.2] * self.num_channels)
self.recording_1.set_property(key="custom_property_z", values=[0.3] * self.num_channels)
self.recording_1.set_property(key="custom_property_1", values=["value_1"] * self.num_channels)
self.recording_2.set_property(key="custom_property_2", values=["value_2"] * self.num_channels)
add_electrodes(recording=self.recording_2, nwbfile=self.nwbfile)
add_electrodes(recording=self.recording_1, nwbfile=self.nwbfile) Then I would expect the order of custom properties to be
I think this is still the order but within (iii) the custom ones will be added in the order
This is a good point, but I think we should also make sure we are adding columns (i) -> (ii) -> (iii) and then there could be an optional argument that could change this order, but as you pointed out what should be the behavior when there are 2 or more recording properties that have different columns?
|
I think this behavior and all concerns could be abated if we use the Then we can always take the order in that list as the defining order to write, including 'reserving' a column by adding it and populating default 'empty' values for that data type even if it doesn't exist in the first recording extractor triggering the call to |
I'm not sure how to do this, I started refactoring to have the electrodes metadata populated (see e1f2c6c) but I think I'll need help to rewrite current logic. I only managed to do it for now when adding properties by columns (see the test in 403b63a). For the Turner data these changes achieve the ordering of "custom" attributes as they appear in the electrodes metadata: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #793 +/- ##
==========================================
+ Coverage 91.23% 91.26% +0.03%
==========================================
Files 121 121
Lines 6309 6331 +22
==========================================
+ Hits 5756 5778 +22
Misses 553 553
Flags with carried forward coverage won't be shown. Click here to find out more.
|
"location", | ||
"group", | ||
"group_name", | ||
"channel_name", | ||
"rel_x", | ||
"rel_y", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"location", | |
"group", | |
"group_name", | |
"channel_name", | |
"rel_x", | |
"rel_y", | |
"channel_name", | |
"group", | |
"group_name", | |
"location", | |
"x", | |
"y", | |
"z", | |
"rel_x", | |
"rel_y", |
The one column we can't control the order of is naturally the id
, but what do you think about this ordering?
Also could you make up some x/y/z values (possibly using some recent real life examples as a good baseline) to match the location?
Having the tests is a good first start I see you have some for the single extractor case, but if you can add some for the multi extractor case you're unsure about (such that the new tests are failing) then it can help by making the problem more well defined (we know what to achieve, just not how to achieve it) At that point I can take a look and suggest a strategy |
Resolve #792