New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-24585: Update policies in obs_lsst to work with visualizeVisit.py #217
Conversation
Issues were noticed when trying to run showCamera, which produced a seperate file for each detector (rather than one for the entire focal plane). This was also true for pipe_drivers' visualizeVisit.py whose purpose is to create a full focal plane view of a given visit in a single fits file. As such, these templates should not be subdivided further than visit (i.e. omit divisions and any references to raftName, detectorName, and detector). The above remedies the "one file created per detector issue", but there was still an issue with the fact that the levels were not set, such that a full focal plane file was created, but with data from only a single detector in it (and there was a loop over each detector with the file overwritten with each iteration showing the single detector data at each step). The levels, their defaults, and, in particular, the specification of the dataId keys that are NOT relevant to a particular level, are updated here.
The comCamMapper.yaml file was (almost) entirely redundant with the templates already (and indentically) defined in the default lsstCamMapper.yaml. The few exceptions were instances of %(expId)d in the former that are actually %(expId)013d in the latter (e.g. for dcrDiff_diaSrc). I do believe there was an oversight in the 08d -> 13d conversion for the expId length of commit: b2b3add such that they are indeed supposed to have indentical templates and hence the removal of the comCamMapper.yaml override file is the correct thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if @timj could take a quick look just to give a 👍 to the visit → expId
stuff in the tests that's all that's necessary really. Everything else looks good I think (other than my comments on ComCam, which are probably fine too).
@@ -1,468 +0,0 @@ | |||
needCalibRegistry: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this whole file been deleted? Have you set up ComCam to use the same mapper as LSSTCam? I think that should work in principle, and perhaps it inherits from that by default, but just wanted to check that this deletion was deliberate and leaves ComCam in a working state.
raw_subsets = (({'level': 'sensor'}, 1), | ||
({'level': 'sensor', 'filter': 'NONE'}, 1), | ||
({'level': 'sensor', 'filter': 'foo'}, 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really in scope of this review, but I thought that we used detector
in obs_lsst (but then again I guess that's vs ccd
in other places, and that this is a test framework question, so I'm sure it's fine really, just a shame we don't have consistency in general.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm not entirely sure how sensor
is supposed to be interpreted. It seems to me it is the canonical name for the sensor
unit to which the various cameras have their own naming conventions: detector
for obs_lsst
, ccd
for obs_subaru
& obs_cfht
, ccd
or ccdnum
(or hdu
?) for obs_decam
, camcol
for obs_sdss
,...
Most of the existing mappers use the word sensor
in referring to a level
, and I followed suit. E.g.
https://github.com/lsst/obs_base/blob/master/python/lsst/obs/base/test/baseMapper.py#L34
https://github.com/lsst/obs_subaru/blob/master/policy/HscMapper.yaml#L17
https://github.com/lsst/obs_decam/blob/master/policy/DecamMapper.yaml#L17
Also, the sensor
level has to be understood generically, since it is used in singleFrameDriver.py
's ArgumentParser
here: https://github.com/lsst/pipe_drivers/blob/master/python/lsst/pipe/drivers/singleFrameDriver.py#L67-L71
As a demonstration, for latiss
:
In [32]: levels = ['sensor', 'detector', 'ccd', 'expId', 'visit', 'filter']
In [33]: butlerDir = "/home/lauren/LSST/obs_lsst/data/input/latiss"
...: butler = dafPersist.Butler(butlerDir)
CameraMapper INFO: Loading exposure registry from /home/lauren/LSST/obs_lsst/data/input/latiss/registry.sqlite3
CameraMapper INFO: Loading calib registry from /home/lauren/LSST/obs_lsst/data/input/latiss/CALIB/calibRegistry.sqlite3
In [34]: for level in levels:
...: subsets = butler.subset("raw", level=level)
...: for subset in subsets:
...: print("Subset dataIds for level {}".format(level), subset.dataId)
Subset dataIds for level sensor {'dayObs': '2018-09-20', 'expId': 3018092000065, 'detector': 0}
Subset dataIds for level detector {'dayObs': '2018-09-20', 'expId': 3018092000065, 'detector': 0}
Subset dataIds for level ccd {'dayObs': '2018-09-20', 'expId': 3018092000065, 'detector': 0}
Subset dataIds for level expId {'dayObs': '2018-09-20', 'expId': 3018092000065}
Subset dataIds for level visit {'expId': 3018092000065}
Subset dataIds for level filter {'expId': 3018092000065}
or for comCam
:
In [35]: butlerDir = "/home/lauren/LSST/obs_lsst/data/input/comCam"
...
Subset dataIds for level sensor {'run': 'unknown', 'raftName': 'R22', 'expId': 3019053000001, 'detectorName': 'S00', 'detector': 0}
Subset dataIds for level detector {'run': 'unknown', 'raftName': 'R22', 'expId': 3019053000001, 'detectorName': 'S00', 'detector': 0}
Subset dataIds for level ccd {'run': 'unknown', 'raftName': 'R22', 'expId': 3019053000001, 'detectorName': 'S00', 'detector': 0}
Subset dataIds for level expId {'run': 'unknown', 'expId': 3019053000001}
Subset dataIds for level visit {'expId': 3019053000001}
Subset dataIds for level filter {'expId': 3019053000001}
Subset dataIds for level tract {'run': 'unknown', 'raftName': 'R22', 'expId': 3019053000001, 'detectorName': 'S00', 'detector': 0}
So, you can see that, when a subset of a given level
is retrieved from butler
, the dataId
s it returns have entries only relevant to that level
, as defined in the camera's mapper, i.e. excludes those listed an not relevant to a particular level (and this hopefully answers you question as to what's happening in the tests below).
In case you're really interested, HSC looks like:
In [3]: butlerDir = "/datasets/hsc/repo/rerun/RC/w_2020_22/DM-25176"
...
Subset dataIds for level sensor {'field': 'DOMEFLAT', 'dateObs': '2014-02-02', 'pointing': 763, 'filter': 'HSC-Z', 'visit': 905850, 'ccd': 38, 'taiObs': '2014-02-02', 'expTime': 30.0}
Subset dataIds for level detector {'field': 'DOMEFLAT', 'dateObs': '2014-02-02', 'pointing': 763, 'filter': 'HSC-Z', 'visit': 905850, 'ccd': 38, 'taiObs': '2014-02-02', 'expTime': 30.0}
Subset dataIds for level ccd {'field': 'DOMEFLAT', 'dateObs': '2014-02-02', 'pointing': 763, 'filter': 'HSC-Z', 'visit': 905850, 'ccd': 38, 'taiObs': '2014-02-02', 'expTime': 30.0}
Subset dataIds for level expId {'field': 'DOMEFLAT', 'dateObs': '2014-02-02', 'pointing': 763, 'filter': 'HSC-Z', 'visit': 905850, 'ccd': 38, 'taiObs': '2014-02-02', 'expTime': 30.0}
Subset dataIds for level visit {'field': 'DOMEFLAT', 'dateObs': '2014-02-02', 'pointing': 763, 'filter': 'HSC-Z', 'visit': 905850, 'taiObs': '2014-02-02', 'expTime': 30.0}
Subset dataIds for level filter {'field': 'DOMEFLAT', 'dateObs': '2014-02-02', 'pointing': 763, 'filter': 'HSC-Z', 'visit': 905850, 'ccd': 38, 'taiObs': '2014-02-02', 'expTime': 30.0}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!
('skyTile', set(['expId', 'run'])), | ||
('filter', set(['expId'])), | ||
('expId', set(['expId', 'run'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be honest, I don't really understand what this was doing before or after really. I guess as long as it's necessary it's fine, but if you could very briefly explain, it would help me learn 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this get sufficiently answered above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you!
No description provided.