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-6614: Add Kron configuration info to metadata #6
Conversation
metadata.add(name + "_minimumRadius", ctrl.minimumRadius); | ||
metadata.add(name + "_enforceMinimumRadius", ctrl.enforceMinimumRadius); | ||
metadata.add(name + "_useFootprintRadius", ctrl.useFootprintRadius); | ||
metadata.add(name + "_smoothingSigma", ctrl.smoothingSigma); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need all of this; the metadata should include the information needed to interpret the results, but it doesn't need to include all of the configuration (and given that it doesn't for other algorithms I don't we want to selectively do that here). I'd recommend just setting "nRadiusForFlux". @PaulPrice, do you concur?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all we want is the information required to reproduce the flux measurement in the catalog from the known shape and Kron radius, and that means just nRadiusForFlux
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuration is already persisted separately (and ought to be retrievable by anyone accessing result datasets just as easily as processing metadata), so I'd generally avoid putting any configuration information into the processing metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with all of you... No configuration, except nRadiusForFlux is something you need to understand the measurements (e.g. plotting the aperture used).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm taking out all the metadat other than nRadiusForFlux.
The Kron photometry algorithm uses parameters that were not being saved in the algorithm metadata. This commit saves the algorithm parameters in the metadata passed to the KronPhotometryAlgorithm constructor.
The constructor for KronPhotometryAlgorithm has been updated to require a PropertySet algorithm metadata object. This commit updates the use of the constructor call to the new standard.
8a08488
to
5d6b4bf
Compare
This commit tests if the Kron photometry algorithm metdata is being set correctly by the existing KronPhotometryTestCase in tests/Kron.py
5d6b4bf
to
1bc52a1
Compare
No description provided.