-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add target_tables
argument to DynamicTable.__init__
#971
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #971 +/- ##
==========================================
+ Coverage 88.57% 88.58% +0.01%
==========================================
Files 45 45
Lines 9400 9395 -5
Branches 2673 2673
==========================================
- Hits 8326 8323 -3
+ Misses 759 758 -1
+ Partials 315 314 -1
☔ View full report in Codecov by Sentry. |
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.
Overall, the approach looks fine to me. However, the specific use I think may be a bit tricky to understand at first, so I think we should make sure to clearly define when to use target_table
vs. when to use the columns
argument of DynamicTable.__init__
.
Thanks for the quick review! |
Co-authored-by: Ryan Ly <[email protected]>
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.
Thanks for the clarifications in the text. The PR looks good to me.
Motivation
In a previous PR, we added the ability to specify the target table for a pre-defined
DynamicTableRegion
column inDynamicTable.__init__
but this was only in generated subclasses ofDynamicTable
. This functionality works well, but could be useful to build in toDynamicTable
in case users want to write their own custom subclass ofDynamicTable
and want to reuse this generic behavior (such as in pynwb and in extensions). Though in PyNWB, we often have a more specific constructor argument for this: https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/src/pynwb/misc.py#L160 and some hacky code inadd_row
to handle setting the target table when the first row is added: https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/src/pynwb/misc.py#L207So I moved the functionality from the custom class generator for generated
DynamicTable
subclasses to the baseDynamicTable
itself.This also has the benefit of fixing #970 when there are multiple chains of hierarchy below
DynamicTable
. Fixes #970.We can optionally adjust pynwb to reuse this behavior in the Units table and elsewhere.
How to test the behavior?
Checklist
ruff
from the source directory.