DM-28368: Fix id check so that all components can use readComponent. #558
Conversation
tests/test_readers.py
Outdated
| @@ -182,18 +182,29 @@ def checkExposureFitsReader(self, exposureIn, fileName, dtypesOut): | |||
| self.assertEqual(exposureIn.getMetadata().toDict(), reader.readMetadata().toDict()) | |||
| self.assertWcsAlmostEqualOverBBox(exposureIn.getWcs(), reader.readWcs(), self.bbox, | |||
| maxDiffPix=0, maxDiffSky=0*degrees) | |||
| self.assertWcsAlmostEqualOverBBox(exposureIn.getWcs(), reader.readComponent('SKYWCS'), | |||
There was a problem hiding this comment.
I recommend using ExposureInfo.KEY_WCS, if only to minimize cargo-culting of magic values.
tests/test_readers.py
Outdated
| self.assertEqual(exposureIn.getInfo().getVisitInfo().getExposureTime(), | ||
| reader.readVisitInfo().getExposureTime()) | ||
| point = Point2D(2.3, 3.1) | ||
| wavelengths = np.linspace(4000, 5000, 5) | ||
| self.assertFloatsEqual(exposureIn.getInfo().getTransmissionCurve().sampleAt(point, wavelengths), | ||
| reader.readTransmissionCurve().sampleAt(point, wavelengths)) | ||
| # Note: readComponent('TRANSMISSION_CURVE') does not work. |
There was a problem hiding this comment.
Nothing obviously jumps out at me; it should behave the same as every other component. Are you getting None, or some kind of error?
For None, the only possibility I can think of is that the logic in https://github.com/lsst/afw/blob/master/src/image/ExposureFitsReader.cc#L360-L363 is failling. Can you manually inspect the file and see if it has an ARCHIVE_ID_TRANSMISSION_CURVE keyword, and that it matches the old-style TRANSMISSION_CURVE_ID?
There was a problem hiding this comment.
It returns a generic Storable that hasn't been casted to a TransmissionCurve.
There was a problem hiding this comment.
I think that means there's a bug in TransmissionCurve's pybind11 wrapper. The formal C++ type is Storable, but that shouldn't matter in Python.
There was a problem hiding this comment.
I don't know how this works, and looking at the transmissionCurve.cc it seems okay to me compared to others like it. Any hints greatly appreciated!
There was a problem hiding this comment.
Sorry, I can't figure it out, either. I think you may need @TallJimbo to weigh in, he knows more about the guts of pybind11 than I do.
Another possibility is that, because the type checking in the current implementation of ArchiveReader::getComponent is sloppier than it could be, the component really is a Storable but not of a type known to Python. I don't know of any such type, though.
There was a problem hiding this comment.
If you mean the Doxygen docs, then yes.
There was a problem hiding this comment.
I'd also add an implementation note to TransmissionCurve that says most objects are of a subclass, and that this may affect pybind11 behavior. It may, as @TallJimbo says, be a good pattern in general, but it's very unusual in our code.
There was a problem hiding this comment.
I added 53cd70a for the readComponent note, but I need explicit direction on these other doc edits since it's further in the weeds of c++ that I don't have the vocabulary/knowledge to describe what is going on here.
There was a problem hiding this comment.
I'm not sure what to say for Storable (it's not really an issue specific to that class), but I can push a blurb for TransmissionCurve.
There was a problem hiding this comment.
That would be awesome, thanks! (And I agree, this isn't a Storable issue per-se, though I can see that the doc string needs to be modified to say that these pybind11 wrappers are non-trivial in some cases?)
Note that the TransmissionCurve is special in some way and does not work with the current implementation.
erykoff
force-pushed
the
tickets/DM-28368
branch
from
54b4378
to
9a35beb
Compare
src/image/ExposureFitsReader.cc
Outdated
| @@ -401,6 +401,10 @@ class ExposureFitsReader::ArchiveReader { | |||
| * | |||
| * @throws pex::exceptions::NotFoundError Thrown if the component is | |||
| * registered in the file metadata but could not be found. | |||
| * | |||
| * @note When accessing via %pybind11, components with derived subclasses, such | |||
There was a problem hiding this comment.
I'd say "from Python"; I'm not sure all our Python developers think of themselves as pybind11 users.
EDIT: sorry for putting my original comment on the commit rather than the PR!
erykoff
force-pushed
the
tickets/DM-28368
branch
from
53cd70a
to
7d3fab7
Compare
|
@kfindeisen Thanks! So is this good to go when Jenkins is green? |
Note that the TransmissionCurve is special in some way and
does not work with the current implementation.