-
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
Refactor DTR warning #917
Refactor DTR warning #917
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #917 +/- ##
=======================================
Coverage 88.34% 88.34%
=======================================
Files 45 45
Lines 9292 9310 +18
Branches 2653 2658 +5
=======================================
+ Hits 8209 8225 +16
- Misses 765 766 +1
- Partials 318 319 +1
☔ View full report in Codecov by Sentry. |
@rly this slipped by. I'll review today. |
# 1) if the table was read from a written file, no need to validate further | ||
p = self.table | ||
while p is not None: | ||
if p.container_source is not None: |
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.
@rly what's the difference between parent and container source? Is the source meant to be the root?
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.
This checks if the table being used in the DTR has a source, if it does then pass, if not then it will check the parent for a source (and so on)
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.
Container_source ==> path to the file ==>Does zarrio set container_source (confirm?)
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.
container_source is set in BuildManager.build
. ZarrIO deals with read/write of Builders, but the logic for creating Containters is the same as in HDMF, i.e., HDMF is setting the container_source and the logic to do so is independent of whether HDF5IO or ZarrIO is being used.
table_ancestor_ids = [id(x) for x in self.table.get_ancestors()] | ||
self_ancestor_ids = [id(x) for x in self.get_ancestors()] | ||
|
||
if set(table_ancestor_ids).isdisjoint(self_ancestor_ids): |
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.
This will check to see of the table and the DTR have some shared ancestor, aka check that its in the same file. If not then it'll be an external link and a warning will be thrown but it will be allowed to write. Correct?
Motivation
Currently
AbstractContainer
performs a check fortype(child).__name__=='DynamicTableRegion'
. While this currently works, it is not ideal to have DTR-specific code inAbstractContainer
and it only matches based on class name, not class type. This code is harder to find and does not work well for subclasses of DynamicTableRegion.In addition, the check is not comprehensive in covering the possible situations in which it is useful. It only checks whether the referred-to table has a parent. The referred-to table may have a parent, but that parent may not have a parent. e.g. in NWB, a PlaneSegmentation may have an ImageSegmentation parent and that ImageSegmentation parent may have a ProcessingModule parent but that ProcessingModule may not have been added to the NWBFile.
See #891 and #885
Checklist
ruff
from the source directory.