DM-20763: Add initial support for Gen3 Butler to obs_decam #164
Conversation
|
|
||
| First write a test case, derived from `~lsst.daf.butler.instrument_tests.InstrumentTests` and `lsst.utils.tests.TestCase`, defining ``self.data`` and ``self.instrument`` in ``setUp``. Run this test via ``pytest -sv tests/test_newObs.py``: all of the tests should fail. | ||
|
|
||
| Next, at minimum, override these abstract methods: `~lsst.daf.butler.Instrument.getName`, `~lsst.daf.butler.Instrument.getCamera`. |
There was a problem hiding this comment.
I'm not sure we want to make getCamera part of the Instrument base class API, as it implies that there is a nominal camera that can be retrieved without providing a calibration repository. That's true right now for all cameras, of course, because it's all Gen2 supports. But I'm not sure it will be true forever.
That said, I'm not sure it won't be true forever, and maybe refactoring all of the cameras if/when we make that change isn't a big deal. But that's the reason why getCamera was only defined on HyperSuprimeCam, not Instrument, before.
There was a problem hiding this comment.
Yup, I knew that context going in. The problem is that we have to actually choose whether to make getCamera part of the Instrument API or not at all. Right now, we need it in order to have FitsRawFormatter.getDetector() work (which we need for the new SkyWcs code), so I've made it a proper part of the API, so that getDetector works in the other Cameras.
Do you have a different approach you would recommend to deal with this until versioned calibration repos become available?
There was a problem hiding this comment.
I do wonder if we have some guidance as to what getName should return. ObservationInfo defines an instrument name and a telescope name. How should this name relate to the translator name?
There was a problem hiding this comment.
Good question: in the tests I've written on this ticket, I'm assuming that getName()==camera.getName(), which should probably also equal observationInfo.instrument_name.
There was a problem hiding this comment.
I think I'd like to be pragmatic rather than prescriptive here: I think it's highly desirable for unambiguous, short instrument-only names to be acceptable for Instrument.getName() (e.g. "HSC") even if in other contexts we need longer qualified names (e.g. "LSST-LATISS"). We may well need to add something like ObservationInfo.full_name to match this, but I don't recall if we currently have anything in e.g. RawIngestTask that requires equivalence between ObservationInfo's names and Instrument's name (but I bet @parejkoj knows now!)
On the getCamera question, let's just live with having it in getInstrument now.
parejkoj
force-pushed
the
tickets/DM-20763
branch
from
4d25fc4
to
9f462c1
Compare
|
|
||
| First write a test case, derived from `~lsst.daf.butler.instrument_tests.InstrumentTests` and `lsst.utils.tests.TestCase`, defining ``self.data`` and ``self.instrument`` in ``setUp``. Run this test via ``pytest -sv tests/test_newObs.py``: all of the tests should fail. | ||
|
|
||
| Next, at minimum, override these abstract methods: `~lsst.daf.butler.Instrument.getName`, `~lsst.daf.butler.Instrument.getCamera`. |
There was a problem hiding this comment.
I think I'd like to be pragmatic rather than prescriptive here: I think it's highly desirable for unambiguous, short instrument-only names to be acceptable for Instrument.getName() (e.g. "HSC") even if in other contexts we need longer qualified names (e.g. "LSST-LATISS"). We may well need to add something like ObservationInfo.full_name to match this, but I don't recall if we currently have anything in e.g. RawIngestTask that requires equivalence between ObservationInfo's names and Instrument's name (but I bet @parejkoj knows now!)
On the getCamera question, let's just live with having it in getInstrument now.
There was a problem hiding this comment.
Again, only minor comments. For some reason I think the PR I'm looking at isn't showing some of @timj's comments, so I may have duplicated some of his and/or failed to reply. I'll go back and look again after I hit the submit button.
parejkoj
force-pushed
the
tickets/DM-20763
branch
5 times, most recently
from
b08a603
to
b5bf6a4
Compare
|
@timj @TallJimbo : can you please take another look at |
parejkoj
force-pushed
the
tickets/DM-20763
branch
3 times, most recently
from
5bd7a72
to
aa213bd
Compare
TallJimbo
force-pushed
the
tickets/DM-20763
branch
from
aa213bd
to
9079f56
Compare
Filters are immutable, and thus the dataclass is frozen. These definitions can be used by both gen2 and gen3, and should help manage that transition.
parejkoj
force-pushed
the
tickets/DM-20763
branch
from
6be7926
to
9f1ca77
Compare
Since we define the filters in init, we can then use always use them in `makeFilter`. Add filterDefinitions property to formatter test
These tests were moved from obs_subaru/tests/test_ingest.py so that other obs packages can use them.
parejkoj
force-pushed
the
tickets/DM-20763
branch
from
9f1ca77
to
6ddf96d
Compare
No description provided.