Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: obs_decam
-
Labels:
-
Story Points:6
-
Epic Link:
-
Sprint:Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c, Alert Production S17 - 12, Alert Production S17 - 1, Alert Production S17 - 2, Alert Production S17 - 3
-
Team:Alert Production
Description
When ingesting calibration files with obs_decam, the current configuration in the mapper policy is to save in the registry the path from the current working directory to the ingested files. This is very fragile and can only work if ingestCalibs.py is run from within the calibration repository itself, and there are numerous ways in which this can still fail. The ingest script and mapper policy file should be updated to specify a path template, and to symlink or copy the calibration files into the repository as is done for ingesting images.
Attachments
Issue Links
Activity
I think the proper fix to this issue is DM-6722.
Paul Price
added a comment - I think the proper fix to this issue is DM-6722 . I don't see how DM-6722 proposes a fix for this issue?
As Paul Price suggested on github, I think having paths defined relative to the calib root would be a significant improvement over the current situation. However, I don't quite understand why we are using paths at all instead of templates. This seems to break the pattern established for the rest of the non-calib repositories.
Colin Slater
added a comment - I don't see how DM-6722 proposes a fix for this issue?
As Paul Price suggested on github, I think having paths defined relative to the calib root would be a significant improvement over the current situation. However, I don't quite understand why we are using paths at all instead of templates. This seems to break the pattern established for the rest of the non-calib repositories. I should have been a bit clearer. Since it's only the community pipeline-produced calibs that use paths, I had assumed it would make sense to have them mirror the LSST-produced calib templates. E.g.,
cpBias: {
|
template: "%(path)s[%(ccdnum)d]"
|
...
|
becomes
cpBias: {
|
template: "cpBIAS/%(calibDate)s/BIAS-%(calibDate)s.fits[%(ccdnum)d]"
|
...
|
Is there anything I'm missing that would make this not work?
Colin Slater
added a comment - I should have been a bit clearer. Since it's only the community pipeline-produced calibs that use paths, I had assumed it would make sense to have them mirror the LSST-produced calib templates. E.g.,
cpBias: {
template: "%(path)s[%(ccdnum)d]"
...
becomes
cpBias: {
template: "cpBIAS/%(calibDate)s/BIAS-%(calibDate)s.fits[%(ccdnum)d]"
...
Is there anything I'm missing that would make this not work? Would you be willing to review this please, Hsin-Fang Chiang? It should be relatively quick.
A few comments for now:
- Coordinated change in testdata_decam is needed, for example the calibRegistry.sqlite3 there likely needs to be regenerated.
- How about DECam defects? Currently its template isn't changed in your PR. Do you intend to continue using path for defects?
- Similar to what Paul Price mentioned, does this affect ingesting LSST-generated calibration data? Sorry a lot more manual tests may be needed than usual, as very little of this code is tested automatically (if at all). (It would be great to add automatic tests somehow, but I understand if you think that's beyond this ticket. )
- Documentation would be nice. obs_decam README will need updates if this results in any operational changes.
Hsin-Fang Chiang
added a comment - A few comments for now:
Coordinated change in testdata_decam is needed, for example the calibRegistry.sqlite3 there likely needs to be regenerated.
How about DECam defects ? Currently its template isn't changed in your PR. Do you intend to continue using path for defects?
Similar to what Paul Price mentioned, does this affect ingesting LSST-generated calibration data? Sorry a lot more manual tests may be needed than usual, as very little of this code is tested automatically (if at all). (It would be great to add automatic tests somehow, but I understand if you think that's beyond this ticket. )
Documentation would be nice. obs_decam README will need updates if this results in any operational changes.
I think this is truly ready for review now!
I did need to update the registry in testdata_decam so all the tests would pass. Updating defect ingestion is outside the scope of this ticket; see DM-5467. I added a note to the obs_decam README about the new mode=link option/suggestion for biases and flats. The only potential backwards compatibility issue I see is with obs_decam users who already have a calibRegistry with the old "path" column in the flat and bias tables. I was thinking I would post on Community about this once the review is complete.
Successful Jenkins builds #22435
https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py2/22435/console
https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py3/22435/console
https://ci.lsst.codes/job/stack-os-matrix/label=osx,python=py2/22435/console
https://ci.lsst.codes/job/stack-os-matrix/label=osx,python=py3/22435/console
The code changes look okay to me. Thanks for the hard work.
For future reference, defect continues to use path. And the paths are relative paths from where the command is executed. 
I don't mind using path; after all obs_base does say it's okay to use path. I was hoping the path could be relative to the calib directory, as Paul Price commented on the PR. But I understand it's already a lot in this ticket and you want to defer that to later.
Hsin-Fang Chiang
added a comment - The code changes look okay to me. Thanks for the hard work.
For future reference, defect continues to use path . And the paths are relative paths from where the command is executed.
I don't mind using path ; after all obs_base does say it's okay to use path . I was hoping the path could be relative to the calib directory, as Paul Price commented on the PR. But I understand it's already a lot in this ticket and you want to defer that to later. 
This and
DM-6722are different but I could see one may potentially affect the other. Linking them just in case..