DM-34919: Allow the generic butler tests to work with gen2 and gen3 #419
Conversation
| @@ -130,7 +133,17 @@ def setUp_butler_get( | |||
| raw_header_wcs=raw_header_wcs, | |||
| ) | |||
|
|
|||
| def _require_gen2(self): | |||
| if isinstance(self.butler, Butler): | |||
| self.skipTest("This test requires daf_persistence Butler, not daf_butler Butler.") | |||
There was a problem hiding this comment.
Should we just remove these tests instead?
There was a problem hiding this comment.
I'm inclined to leave the gen2 tests there until we rip gen2 out of obs_base itself.
| def test_exposureId_bits(self): | ||
| self._require_gen2() |
There was a problem hiding this comment.
I think we should just remove this test.
| @@ -139,13 +152,17 @@ def _test_exposure(self, name): | |||
| self.skipTest("Skipping %s as requested" % (inspect.currentframe().f_code.co_name)) | |||
| exp = self.butler.get(name, self.dataIds[name]) | |||
|
|
|||
| exp_md = self.butler.get(name + "_md", self.dataIds[name]) | |||
| md_component = ".metadata" if self._is_gen3() else "_md" | |||
There was a problem hiding this comment.
Are you doing the _is_gen3 thing to deal with obs packages that haven't moved their tests to gen3 yet?
There was a problem hiding this comment.
At the time I thought I had to do it because I assumed other obs packages used the obs_base tests. It seems like obs_lsst is the only one (other than possibly other external users). The butler_tests can work out from their own butler that it is gen2 vs gen3. At this point I'm inclined to leave the gen2 support in until all four of our obs packages drop gen2 and then rip it all out.
python/lsst/obs/base/butler_tests.py
Outdated
| self.assertEqual(exp.getDetector().getSerial(), self.butler_get_data.detector_serials[name]) | ||
| detector = exp.getDetector() | ||
| if detector: | ||
| # Some calibration files are missing the detector. |
There was a problem hiding this comment.
When does this happen? I thought we could always assume that the detector exists in LSST files?
There was a problem hiding this comment.
Some TS8 bias frames somehow don't have the detector. For all I know they were created by the camera team. I have no idea how easy it is to locate a newer TS8 example bias or if it should just be deleted.
| The arguments to add. | ||
| """ | ||
| kwargs = {} | ||
| if isinstance(self.butler, Butler): |
There was a problem hiding this comment.
Shouldn't this use _is_gen3?
There was a problem hiding this comment.
Multiple inheritance makes it all a bit murky as to whether it will find it. I felt it was clearer for this temporary fix to do the explicit test in this file.
| @@ -162,23 +168,28 @@ def test_map_metadata_data(self): | |||
| ) | |||
|
|
|||
| def test_keys(self): | |||
| self._check_mapper() | |||
There was a problem hiding this comment.
I'd argue that we should just remove mapper_tests and mapper_tests.MapperTests from ObsTests.
There was a problem hiding this comment.
Yeah. I kept changing my mind on this and decided to keep them around until obs_decam and obs_cfht stop using gen2 and obs_base can be completely cleaned out.
| self.skipTest("This test requires daf_persistence Butler, not daf_butler Butler.") | ||
|
|
||
| def _is_gen3(self): | ||
| if isinstance(self.butler, Butler): |
There was a problem hiding this comment.
Is it worth flipping this around to be is_gen2 and call isinstance(self.butler, lsst.daf.persistence.Butler) instead, so that we have a very clear thing to remove when we finally remove daf_persistence?
There was a problem hiding this comment.
_is_gen3 is going to be deleted when we clear out.
|
After I had made it compatible with gen2 and gen3 I realized that no obs package other than obs_lsst actually uses it (at least ours). In theory we could delete all these tests from obs_base. I'm no longer convinced that these tests cover anything more than we already test with the butler ingest code (where we ingest and then retrieve the raw and get components). Adding some extra queries in obs_lsst that use day_obs/seq_num may well cover everything. The only extra thing these tests do is allow a bias to be retrieved for a couple of instruments. |
timj
force-pushed
the
tickets/DM-34919
branch
3 times, most recently
from
789635b
to
e6dad37
Compare
The mapper tests now disable themselves if no mapper is available.
It won't work for lsst data which has separate amps and whether it works or not is dependent on the raw formatter in use. Easier to remove the test for now.
|
Originally, these tests were going to form the backbone for obs_decam and obs_subaru tests, too, but we were never able to complete that work. If you think we get good coverage from elsewhere, we might be able to remove them all. Maybe make a ticket to assess that after gen2 is gone (so we don't have as many places to look at coverage)? |
The mapper tests now disable themselves if no mapper is available.
Required by lsst/obs_lsst#402
Checklist
doc/changes