DM-38054: Add DataCoordinate attribute access to DimensionRecords. #792
Conversation
There was a problem hiding this comment.
Looks good to me. Can we also have an explicit test for the common usage? In test_dimensions.py there is a line:
self.assertEqual(dataId.timespan, dataId.records["visit"].timespan)can we add a line underneath it as:
self.assertEqual(dataId.timespan, dataId.visit.timespan)?
| try: | ||
| return self._record(name) | ||
| except KeyError: | ||
| raise AttributeError(name) from None |
There was a problem hiding this comment.
It wouldn't hurt to also have a test on an unexpanded dataId to make sure it raises AttributeError? Then I imagine the follow up will be that people will want to distinguish between an attribute that is failing because it's not part of the dataId vs one that is failing because the records were not available (so maybe the error message should mention the distinction).
TallJimbo
force-pushed
the
tickets/DM-38054
branch
from
b1a7c32
to
812cb31
Compare
Without this, the default pickle implementation apparently tries to do getattr on instances before they're fully constructed while catching the exceptions. And that leads to infinite recursion if one writes a __getattr__ that involves accessing attributes added at construction.
TallJimbo
force-pushed
the
tickets/DM-38054
branch
from
812cb31
to
05668d9
Compare
Codecov ReportBase: 85.51% // Head: 85.54% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #792 +/- ##
==========================================
+ Coverage 85.51% 85.54% +0.02%
==========================================
Files 266 266
Lines 35087 35137 +50
Branches 7364 7377 +13
==========================================
+ Hits 30004 30057 +53
+ Misses 3768 3765 -3
Partials 1315 1315
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
|
||
| See also | ||
| -------- | ||
| :ref:`lsst.daf.butler-dimensions_data_ids` |
There was a problem hiding this comment.
I tried building the docs but I didn't see this section included. May want to consider embedding it into the Notes section instead.
There was a problem hiding this comment.
It's there (or at least it is for me) - it's a little call-out box above the Notes section, unlike the way it appears in the source.
There was a problem hiding this comment.
Ah. Yes. I see it now I know what I'm looking for. Sorry.
Checklist
doc/changes