* Store polarization axes in response as type PolarizationAxis#518
Draft
jdbuhler wants to merge 2 commits intocositools:developfrom
Draft
* Store polarization axes in response as type PolarizationAxis#518jdbuhler wants to merge 2 commits intocositools:developfrom
jdbuhler wants to merge 2 commits intocositools:developfrom
Conversation
* When we load a polarization FDR, convert the type of the Pol axis to a PolarizationAxis if it is not already, using the provided pa_convention argument * Add PolarizationAxis support to RspConverter assuming that the convention will be added to the MEGAlib axis name; since we now have to parse axis names, get the axis unit from the axis name instead of assuming it * fix polarization_axis to correctly read the convention from the version stored in HDF5 * improve registered name support for polarization conventions, so that we don't have to iterate through a map to get a convention's name * change test_polarization_response in test_data to use a PolarizationAxis for its Pol axis * change the polarization fitting test cases so they do not pass a convention, since we now supply that information in the response file
Collaborator
|
Thanks @jdbuhler
If the input file has the old format, and the user does not specify the convention, can we raise an exception instead? I think that's the current behavior, but even if it's not, it looks safer. |
…ne and none is persent in file
Contributor
Author
|
I changed the loading behavior for the FDR to behave as you specify. |
Collaborator
|
Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This draft PR changes the FullDetectorResponse to store Pol axes using the PolarizationAxis type. The principal motivation of this change is to avoid having to specify a polarization convention every time we open the response -- the PolarizationAxis stores its own convention.
If a response is loaded in HDF5 format and has a Pol axis that is not in PolarizationAxis (the old, legacy format), we convert it to a PolarizationAxis using the convention passed to FDR.open() (raising an exception if none is given). Hence, every polarization FDR, old or new, now presents the Pol axis as a PolarizationAxis.
Polarization responses can be specified in .rsp format by adding the convention to the MEGAlib axis name, e.g.,
AN "Polarization Angle [deg] [relativez]"
This change requires us to parse the axis name to get the convention out; since we are already parsing the name, we now also get the units of all axes from their MEGAlib names rather than having a fixed list in RspConverter.
The test_polarization_response files in test_data have been converted to the new format, and the polarization fitting test cases have been changed to stop passing pa_convention, assuming that convention is now available from the response.
This branch currently passes all the response-related tests, in particular tests of RspConverter, but fails the polarization_ASAD and polarization_stokes tests. While I tried to change these two fitting methods to use the polarization convention from the file, getting them to work will require more substantial changes that are beyond my current ability. The problem is that the axis edges and centers returned by a PolarizationAxis are now PolarizationAngles, not scalar angles, and this breaks a great deal of code that assumes that they are scalar angles. I've left these methods in their original versions, rather than commit changes that might be broken, so someone else can figure out how to adapt them.