Skip to content
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-27169: Use FilterLabel in Exposure/ExposureInfo #551

Merged
merged 10 commits into from Dec 9, 2020

Conversation

kfindeisen
Copy link
Member

This PR replaces both the internal state and the persistence format of Exposure to use FilterLabel instead of Filter. Accessors for Filter remain for backwards-compatibility, but are re-implemented in terms of FilterLabel. I've also done some refactoring of the generic component code that should make it easier to apply in the future.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A handful of questions/comments, but this all looks quite good.

The logic for building a FilterLabel from a Filter is even more tortured than I'd feared, but I'm glad you were able to get it sorted out.

src/image/ExposureFitsReader.cc Show resolved Hide resolved
src/image/ExposureFitsReader.cc Show resolved Hide resolved
src/image/ExposureFitsReader.cc Show resolved Hide resolved
src/image/ExposureFitsReader.cc Show resolved Hide resolved
@@ -223,12 +413,9 @@ class ExposureFitsReader::ArchiveReader {

// Not safe to call getAll if a component cannot be unpersisted
// Instead, look for the archives registered in the metadata
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how you reworked this with readComponent, is this comment still necessary?

Copy link
Member Author

@kfindeisen kfindeisen Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so? If I could call getAll, I wouldn't need to manually build up a map of results.

Add methods and tests analogous to the old Filter support. FilterLabel
automatically persisted through generic component code.
This change gives ArchiveReader the flexibility to read old
ExposureInfo components using either the legacy system or the
generic component system.
This change makes it possible to read one generic component without
needing to iterate over the rest.
The code lets Exposure files based on Filter be read as Exposures
with a FilterLabel. Filter is also read because some Exposure
methods still need it.
The new code can take either a filter or a name as input.
Separate Filter storage removed from ExposureInfo; now backed by
(generic) FilterLabel.
Filters are now stored only as ExposureInfo components. Add
backwards-compatibility test for ExposureInfo version 2.
Other Stack code assumes Filter.getName() returns the afw_name when
it's needed to disambiguate filters (i.e., HSC-R2 and HSC-I2).
@kfindeisen kfindeisen merged commit e3f3753 into master Dec 9, 2020
@kfindeisen kfindeisen deleted the tickets/DM-27169 branch December 9, 2020 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants