Details
-
Type:
Bug
-
Status: Invalid
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw, meas_base, pipe_tasks
-
Labels:None
-
Team:Architecture
Description
In upgrading cftisio to a newer version, test_exposure.py fails in testing the read output:
https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/30666/tests
DM-21989 will be blocked by this.
The particular part of the test that fails seems to have been exposed by work done in DM-17982 but it's likely the underlying issue is due to a change of serialization in cfitsio.
In DM-20864 it was also discovered that in pipe_tasks, the metadata for "base_CircularApertureFlux_radii" seems to have been persisted in uppercase, so the unit test in tests/nopytest_test_coadds.py would fail. The hack to get past that was to uppercase the key.
This all seems possibly related to HIERARCH handling in CFITSIO, in that it's likely some keywords are persisted to a file as uppercase, so when retrieving information from that file the key had to be uppercased. I suspect this may possibly be related to the previous mentioned issue in afw.
Attachments
Issue Links
Activity
The second issue is actually in meas_base in ApertureFlux.cc but just shows up in pipe_task, exposed in nopytest_test_coadds.py. The issue is that the aperture flux radii is lower cased. I think there's some metadata name wrangling that goes on depending on which subclass it.
I think that key maybe needs to be rethought and upper-cased. I don't have any other suggestion on the naming or handling of it.
This issue is likely to occur in other parts of code putting metadata into PropertySet. I don't have an idea how widespread that is.
I worry that such solutions tie our code even closer to FITS. If we are going to do that, we might as well be explicit about it.
Our continued use of HIERARCH in this manner also continues to worry me. It is supported by cfitsio, but its original design was not intended to be used as a "long keyword" convention, and cfitsio's use in this manner is one interpretation of the convention document: https://fits.gsfc.nasa.gov/registry/hierarch/hierarch.pdf
For the record, here are a couple relevant Community posts where issues related to this have been discussed in the past:
https://community.lsst.org/t/fits-and-lowercase-header-keys/1184/3
https://community.lsst.org/t/safer-handling-of-long-fits-keywords/456/12
Thanks for the link. That was not a side of PropertySet I'd had to deal with before.
On the ExposureInfo side, the creation of header keywords based on component names was always a hack to keep compatibility with the old Exposure format (where the use of FITS is, explicitly, part of the Exposure API). Assuming that Jim Bosch still intends to switch over to a new persistence framework, we will likely have to change the FITS format anyway, and the hope was that we could then store component names in a more natural and less structural way. That's why I'm fine with requiring upper case for now.
EDIT: Okay, we could implement the KEY_ half of KEY_/VAL_ in ExposureInfo right now, and keep round-tripping of lowercase component names. But that would make ExposureInfo's format even more elaborate, and I wouldn't necessarily trust the result.
Krzysztof Findeisen
added a comment - - edited Thanks for the link. That was not a side of PropertySet I'd had to deal with before.
On the ExposureInfo side, the creation of header keywords based on component names was always a hack to keep compatibility with the old Exposure format (where the use of FITS is, explicitly, part of the Exposure API). Assuming that Jim Bosch still intends to switch over to a new persistence framework, we will likely have to change the FITS format anyway, and the hope was that we could then store component names in a more natural and less structural way. That's why I'm fine with requiring upper case for now.
EDIT: Okay, we could implement the KEY_ half of KEY_ / VAL_ in ExposureInfo right now, and keep round-tripping of lowercase component names. But that would make ExposureInfo 's format even more elaborate, and I wouldn't necessarily trust the result.
I'd be happy to deal with the ExposureInfo side of things. I'm not very familiar with pipe_task or measurement plugins.
I think banning lower case names (in ExposureInfo.setComponent) is the most forward-compatible solution; if we make ExposureInfo keys case-insensitive, client code may start relying on it and we won't be able to change it later.