Skip to content

Update glows_l1b_data.py, degrees=False correction #2944

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.

Sign up for GitHub

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

Open
mstrumik wants to merge 3 commits into
base: dev
Choose a base branch
from
+118 −3
Open

Update glows_l1b_data.py, degrees=False correction #2944

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e1a9a9
Update glows_l1b_data.py
mstrumik Apr 9, 2026
50d6f6e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 9, 2026
dc8a80f
Finish fixing GLOWS bug where degrees where getting passed to circmea…
subagonsouth Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion imap_processing/glows/l1b/glows_l1b_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,8 @@ def update_spice_parameters(self) -> None:
np.array([0, 0, 1]),
SpiceFrame.IMAP_SPACECRAFT,
SpiceFrame.ECLIPJ2000,
)
),
degrees=False,
Copy link
Copy Markdown
Contributor

@subagonsouth subagonsouth Apr 9, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

@mstrumik mstrumik Apr 9, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@subagonsouth subagonsouth Apr 9, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

@mstrumik mstrumik Apr 9, 2026

Choose a reason for hiding this comment

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

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.

)
# Calculate circular statistics for longitude (wraps around)
lon_mean = circmean(spin_axis_all_times[..., 1], low=-np.pi, high=np.pi)
Expand Down
Loading