DM-36169: ability to read metadata from specific named HDUs #655
Conversation
timj
changed the title
|
Running the ticket in jenkins: |
weatherhead99
force-pushed
the
tickets/DM-36169
branch
from
46a3cbc
to
5c3b746
Compare
| fp.setHdu(hdu); | ||
| using dafPlistPtr = std::shared_ptr<daf::base::PropertyList>; | ||
|
|
||
| namespace detail { |
There was a problem hiding this comment.
Is there a specific reason to use the detail namespace here?
There was a problem hiding this comment.
no, just that that's what I would have done belt and braces when I used to be a pro... is there a thing we do in this codebase for "small function that is only used internally here, isn't exposed in any type of deliberately accessible linkage"? This is only here to save a few duplicate lines below, but it probably costs more in compile time idk
|
|
||
| # now try to load a non-existent named HDU and check that we throw | ||
| with self.assertRaises(lsst.afw.fits.FitsError): | ||
| lsst.afw.fits.readMetadata(testfile, hduName="CORDON_BLEAU") |
There was a problem hiding this comment.
I guess cordon bleu isn't found either ;-)
There was a problem hiding this comment.
quite so ;-). Again, I didn't see any conventions mandated for test values but can change them to whatever.
|
Needs to be re-based, the commits could be squashed. |
Adding an overload for setHdu, which allows to move to a named HDU with a type. A thin wrapper around cfitsio's fits_movnam_hdu.
- Fits.setHdu can now accept hduname argument - expose one of the new readMetadata top level functions which can take a HDU name instead of number
Also includes a sample MEF file with extra headers for testing
weatherhead99
force-pushed
the
tickets/DM-36169
branch
from
5b50ca9
to
0106e6c
Compare
|
now squashed to 3 commits:
|
|
Running this once more jenkins. Looks good to merge if jenkins succeeds. |
|
I just get the below when looking at Jenkins, am I supposed to be logged in via github or using some other credentials or do I just not have permissions etc? |
|
You might need to be member of lsst-dm as well to access jenkins. |
In order to implement the functionality for https://jira.lsstcorp.org/browse/DM-36169 (which requires downstream changes to lsst.afw.obs_lsst), it was suggested by @timj that the correct way to do this may be to add the ability to load metadata from named HDUs into afw.fits. Ths PR adds the suggested functionality by wrapping the cfitsio fits_movnam_hdu function, and adding overrides at both the c++ and python layers.
A test case is also added to test_fits.py to check whether this works, along with minimalist extra fits data for this test to work on.
This is my first patch attempt to LSST DM, so my apologies if anything is out of order here. But also, please be gentle!
(New pull request not from my fork)...