DM-14114: Add interface for camera specialization #31
Conversation
There was a problem hiding this comment.
I've left a few minor comments, but I think this is enough for me to rebase on and start making some progress. You probably have a better idea than I do now of what's missing here in order to get the conversion scripts working (since I've been away from that for a week). If you want to add anything else to the PR before merging, just ping me if you'd like me to take a look.
python/lsst/daf/butler/instrument.py
Outdated
| raise NotImplementedError('Must be specified by derived class') | ||
|
|
||
| physicalFilters = [] | ||
| sensors = [] |
There was a problem hiding this comment.
It doesn't really make sense to me that camera is an instance property while the others are class attributes.
I think I'd probably just make camera a class attribute that's set to None, and document in the class that all that matters is that subclass instances have these attributes; that will let subclasses that need to override them at the instance level while allowing those that don't to override them at the class level.
| dataUnitName : `str` | ||
| Name of the `DataUnit` (e.g. ``"Camera"``). | ||
| values : `dict` | ||
| Dictionary of ``columnName, value`` pairs. |
There was a problem hiding this comment.
Should document that value is itself a dictionary.
There was a problem hiding this comment.
It is? I had assumed it looked something like: {"camera": "HSC", "abstract_filter": "i", "physical_filter": "HSC_i", "visit": 3}. Did you have something else in mind?
There was a problem hiding this comment.
Oh, I was confused by the fact that the argument is called values in the doc but value in the code (and I was looking at the line where you do **value below). Just rename the argument and I think it'll all make sense.
| def addDataUnit(self, unit, replace=False): | ||
| """Add a new `DataUnit`, optionally replacing an existing one | ||
| (for updates). | ||
| def addDataUnitEntry(self, dataUnitName, value): |
There was a problem hiding this comment.
Should there be an abstract version of this in the base Registry class?
There was a problem hiding this comment.
Yes, but at this point it is easier to copy the SqlRegistry API back into Registry after the dust settles.
There was a problem hiding this comment.
Assuming nobody else is working on an alternative Registry implementation yet.
pschella
force-pushed
the
tickets/DM-14114
branch
from
e02d807
to
e2dee12
Compare
No description provided.