DM-33278: Improve storage class conversion #632
Conversation
timj
force-pushed
the
tickets/DM-33278
branch
from
b287294
to
0f09851
Compare
Codecov Report
@@ Coverage Diff @@
## main #632 +/- ##
==========================================
+ Coverage 84.11% 84.12% +0.01%
==========================================
Files 237 237
Lines 30143 30183 +40
Branches 5014 5018 +4
==========================================
+ Hits 25354 25392 +38
- Misses 3645 3646 +1
- Partials 1144 1145 +1
Continue to review full report at Codecov.
|
timj
force-pushed
the
tickets/DM-33278
branch
from
0f09851
to
dca815c
Compare
If a converter ends with () it is assumed to be a method to run on the object to be converted and not a class method or function.
timj
force-pushed
the
tickets/DM-33278
branch
from
68a3b2c
to
9d7a6a4
Compare
doc/lsst.daf.butler/formatters.rst
Outdated
| In the first definition, the configuration says that if a ``PropertySet`` object is given then the ``toDict()`` method can be called on it and the returned value will be a `dict`. | ||
| In the second definition a ``PropertySet`` is again specified but this time the ``from_metadata`` class method will be called with the ``PropertySet`` as the first parameter and a ``TaskMetadata`` will be returned. |
There was a problem hiding this comment.
This difference between () and no () confused me a lot when I first saw it and it was not obvious what it means. I had to read everything more than once to figure it out. Now I think that toDict() is basically the same as lsst.daf.base.PropertySet.toDict (non-bound method). Would it be better to drop () and use non-bound method syntax for symmetry?
There was a problem hiding this comment.
I've been pondering this and the () syntax has the advantage that it doesn't lead to duplicating the full class name on both sides:
lsst.daf.butler.tests.MetricsExample: exportAsDict()vs
lsst.daf.butler.tests.MetricsExample: lsst.daf.butler.tests.MetricsExample.exportAsDictThe latter does work and doesn't require any code changes to make it work without this ticket. Maybe that's a good enough argument for not trying to bother with the more succinct form.
| if self._isCalibration != other._isCalibration: | ||
| return True | ||
|
|
||
| def is_compatible_with(self, other: "DatasetType") -> bool: |
There was a problem hiding this comment.
Do you need to quote DatasetType here?
There was a problem hiding this comment.
I think I'm confused between from __future__ import annotations being enabled and not being enabled. There was some other place where we did have to use strings if the type was used in the context of its own class but maybe that was to do with pydantic.
There was a problem hiding this comment.
I believe we cannot use from __future__ import annotations with pydantic (which is really a shame, and I think it means we should mostly put pydantic classes in their own modules, so we can use it everywhere else). And without __future__.annotations, you do have to quote in places like this.
| if self._isCalibration != other._isCalibration: | ||
| return True | ||
|
|
||
| def is_compatible_with(self, other: "DatasetType") -> bool: |
There was a problem hiding this comment.
Does "compatible" mean compatible for both get and put? Is it symmetrical (X.is_compatible_with(Y) === Y.is_compatible_with(X))?
There was a problem hiding this comment.
It really means self_sc.can_convert(other_sc) -- You have made me realize that ctrl_mpexec is going to have to try to distinguish inputs (so can convert on get to the required type for the Task) and outputs (so can convert the type used in the Task to the required type in the butler).
This reverts commit 8e15cb7. We can do the same thing with a full name of an unbound method.
This removes surprises when someone gets the dataset (there is no need to also add conversion on get), and provides consistent application of parameters.
timj
force-pushed
the
tickets/DM-33278
branch
from
9d7a6a4
to
5287133
Compare
Sometimes a storage class can be defined with a different name but is referring to the same python type.
Checklist
doc/changes