Glows L1b - Fix spin angle offset calculation in histogram flags#3065
Conversation
cd9115d to
cc443fb
Compare
tmplummer
left a comment
There was a problem hiding this comment.
Thanks for taking this ticket on. Couple of questions about the added test.
| assert np.all(hist_data.histogram_flag_array[0, 1397:1437] == 1) | ||
| assert hist_data.histogram_flag_array[1, 1417] == 2 |
There was a problem hiding this comment.
Where did these values come from? I saw the plot showing that this change makes output match the GLOWS JSON, but these asserts are really opaque as to why we are expecting these values to be set this way.
There was a problem hiding this comment.
This assertion was meant to test that uv sources are flagged at these observed spin angles when using the updated calculation for azimuth. We can look into isolating and testing the look direction calculations instead to offer greater clarity to what is being tested.
| @pytest.mark.external_kernel | ||
| @pytest.mark.usefixtures("use_fake_spin_data_for_time") | ||
| @patch("imap_processing.spice.geometry.imap_state") | ||
| def test_process_histogram_calculates_correct_angle_offset( |
There was a problem hiding this comment.
Would there be a way to just test the HistogramL1B.flag_uv_and_excluded method instead of needing to run the entire HistogramL1B processing? It looks like many of the tests in this file are patching attributes of HistogramL1B to get around having to run the full processing.
There was a problem hiding this comment.
We pulled out the calculation of the look vectors in DPS so that we are able to test that in isolation. Does this seem like a reasonable approach?
…od that can be tested in isolation. Removed redundant test.
| ] | ||
| with furnish_kernels(kernels): | ||
| expected_imap_spin_angle_bin_cntr = np.array([0, 90, 180, 270]) | ||
| expected_position_angle_offset_average = 71.5 |
There was a problem hiding this comment.
Can you add a comment on where 71.5 degrees comes from. I a looked at the GLOW algorithm document a bit and can't seem to figure it out.
There was a problem hiding this comment.
This was just an arbitrary value that we had used since the actual input is coming from an upstream calculation. We have updated the variable names to avoid confusion.
| with furnish_kernels(kernels): | ||
| expected_imap_spin_angle_bin_cntr = np.array([0, 90, 180, 270]) | ||
| expected_position_angle_offset_average = 71.5 | ||
| expected_azimuth = np.array([360 - 71.5, 90 - 71.5, 180 - 71.5, 270 - 71.5]) |
There was a problem hiding this comment.
| expected_azimuth = np.array([360 - 71.5, 90 - 71.5, 180 - 71.5, 270 - 71.5]) | |
| expected_azimuth = np.array([360, 90, 180, 270]) - expected_position_angle_offset_average |
tmplummer
left a comment
There was a problem hiding this comment.
Thanks for making this change. This makes the testing clearer to me. I just have some nit-picks on some of the implementation.
tmplummer
left a comment
There was a problem hiding this comment.
Excellent. Thanks for this work!
55bcc7c
into
IMAP-Science-Operations-Center:dev
…P-Science-Operations-Center#3065) * Glows L1b - Fixed bug in how spin angle offset was used when calculating histogram flags (IMAP-Science-Operations-Center#2946) * Glows L1b - Pulled out calculation of DPS look vectors to static method that can be tested in isolation. Removed redundant test. * Glows L1b - Simplified test logic, variables with more descriptive naming --------- Co-authored-by: Menlo Innovations - CAVA Project <harrison@menloinnovations.com>
Fix bug in how spin angle offset is used when calculating histogram flags, addressing issue #2946
Change Summary
Overview
Changed the calculation of the azimuth to use the correct operation (- instead of +) to match equation 29 in the algorithm document.
File changes
Testing
Added test that asserts that the range of spin angles that are flagged for being close to a UV source and being excluded are accurate to the updated equation.