Update glows_l1b_data.py, degrees=False correction#2944
Update glows_l1b_data.py, degrees=False correction#2944mstrumik wants to merge 3 commits intoIMAP-Science-Operations-Center:devfrom
Conversation
By default geometry.cartesian_to_latitudinal() uses degrees=True so one needs to set explicitly degrees=False here to use circmean/circstd later with low=-np.pi, high=np.pi bounds
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
| SpiceFrame.ECLIPJ2000, | ||
| ) | ||
| ), | ||
| degrees=False, |
There was a problem hiding this comment.
Good catch! Based on the definition of spin_axis_orientation_average, I believe that the mean and standard deviation values need to be converted to degrees before setting the values on lines 950-951.
spin_axis_orientation_average
block-averaged spin-axis ecliptic longitude ⟨λ⟩ and latitude ⟨φ⟩ in degrees
It would probably be beneficial to add a specific test that covers this bug. The SDC can help with that if needed.
There was a problem hiding this comment.
Thank you Tim. You are probably right regarding conversion to degrees. I am not able to find any conversion in the code. So either the conversion you suggested is implemented or alternatively geometry.cartesian_to_latitudinal() may have degrees=True but bounds in circmean/circstd will be low=-180, high=180. Am I supposed to propose the changes (next pull request) or it will be handled internally by SDC?
Regarding the testing, I was comparing variable values in CDFs produced by SDC with JSON files generated by GLOWS team and found some discrepancies that led me to this pull request. So there is some testing on the GLOWS-team side related to this bug. Were you thinking about this type of testing or rather unit tests in the SDC code?
There was a problem hiding this comment.
Based on the Scipy documentation for circmean and circstd, it looks like those functions require the inputs be in radians. So, my suggestion is just to convert the values that get stored to degrees. For example, keep your changes and then modify lines below to:
self.spin_axis_orientation_average = np.degrees(np.array([lon_mean, lat_mean]))
self.spin_axis_orientation_std_dev = np.degrees(np.array([lon_std, lat_std]))
Typically, when you get feedback on a PR, you can just make additional changes on the git branch you are working on that resolve the comment or change requested. Commit those changes and push and the PR will automatically pick them up.
For testing, I was thinking about adding a unit test with some input data that specifically tests this issue you have found. Is there some set of input values and expected results that would check that the bug fixed by these changes works?
I have no idea what your coding experience is, but if you need help, it is also possible for me to check out your branch and make changes on it if showing rather than describing would be more helpful.
There was a problem hiding this comment.
Thanks Tim again. I completely agree with you regarding the code lines you proposed. I have limited experience with Github and IMAP code repository, so "check out your branch and make changes on it" solution would be probably the best one.
I am still thinking about a unit test for the bug.
…n and circstd functions Add specific test coverage checking that spin_axis_orientation is now as expected
|
@mstrumik did you create a github issue associated with this PR? |
2 participants
By default geometry.cartesian_to_latitudinal() uses degrees=True so one needs to set explicitly degrees=False here to use circmean/circstd later with low=-np.pi, high=np.pi bounds
Change Summary
Overview
Added degrees=False in geometry.cartesian_to_latitudinal() invocation for consistency with circmean/circstd invoked later
File changes
The purpose is to fix degrees/radians mismatch
Testing