task: Add more SPICE frames for MAG L2 processing and fix issue with SPICE epochs missing#2378
Conversation
…certain epoch, instead NaN matrix is returned
tmplummer
left a comment
There was a problem hiding this comment.
I know this is still just a draft, but I thought that I would leave some comments anyway.
| ): # numpydoc ignore=GL08 | ||
| try: | ||
| return spiceypy.pxform(*arg, **kwargs) | ||
| except spiceypy.utils.exceptions.SpiceNOFRAMECONNECT: |
There was a problem hiding this comment.
This implementation concerns me. I want this function to fail loudly by default rather than just logging an exception level message. I would be more comfortable with adding a keyword argument called something like allow_spice_noframeconnect that defaults to False.
try:
return spiceypy.pxform(*arg, **kwargs)
except spiceypy.utils.exceptions.SpiceNOFRAMECONNECT as e:
if not allow_spice_noframeconnect:
raise(e)
logger.debug(f"Returning NaN rotation matrix due to spiceypy error: {e}")
return np.full((3, 3), np.nan)
There was a problem hiding this comment.
Makes sense, done!
| @pytest.mark.external_kernel | ||
| def test_get_rotation_matrix_no_transformation_defined_for_et(furnish_kernels): | ||
| """Test error handling in get_rotation_matrix().""" | ||
| kernels = [ | ||
| "naif0012.tls", | ||
| "imap_100.tf", | ||
| "imap_sclk_0000.tsc", | ||
| "imap_science_100.tf", | ||
| "sim_1yr_imap_attitude.bc", | ||
| "sim_1yr_imap_pointing_frame.bc", | ||
| "de440s.bsp", | ||
| ] | ||
| with furnish_kernels(kernels): | ||
| # Midnight is not defined in pointing frame | ||
| et = spiceypy.utc2et("2029-01-01T00:00:00.000") | ||
| rotation = get_rotation_matrix(et, SpiceFrame.IMAP_MAG_O, SpiceFrame.IMAP_DPS) | ||
| assert np.isnan(rotation).all() |
There was a problem hiding this comment.
It would be good to also check that the vectorized form works:
# one hour after midnight should have coverage
ets = np.array([et, et + 3600])
rotations = get_rotation_matrix(ets, SpiceFrame.IMAP_MAG_O, SpiceFrame.IMAP_DPS)
assert rotations.shape == (2, 3, 3)
assert np.isnan(rotations[0]).all()
assert np.isfinite(rotations[1]).all()
There was a problem hiding this comment.
I've added this test, and a test for the default allow_spice_noframeconnect value, where we throw an error.
| try: | ||
| l2_data.rotate_frame(frame) | ||
| frames.append(l2_data.generate_dataset(attributes, day)) | ||
| except Exception: |
There was a problem hiding this comment.
Broad exceptions like this are generally bad and in this case all details about what caused the failure to rotate the data to the desired frame is lost.
Are there specific reasons you expect this to fail and can you handle those specific exceptions?
There was a problem hiding this comment.
I've removed this as this change addresses the failures we have seen in the past.
Btw, the logger.exception method does log the exception information automatically (including stacktrace), without having to pass the exception directly:
https://docs.python.org/3/library/logging.html#logging.Logger.exception
The same is true when passing exc_info to other logging methods, like I've done for the debug call.
Thanks Tim, I've implemented the feedback. I'll get it tested in the dev environment with the files that have failed in the past. |
|
This all looks reasonable to me. @mfacchinelli, will you please convert this to a regular PR, as opposed to a Draft, and add @maxinelasp and @greglucas as additional reviewers? |
I can't add reviewers, but the tag should also work! |
maxinelasp
left a comment
There was a problem hiding this comment.
This looks good to me! I am adding the SPICE kernels as required for L2 that I missed (ephemeris kernels). Let me know when you're done testing and ready to merge it. Thanks for the fix!
| DSRF = SpiceFrame.IMAP_DPS | ||
| SRF = SpiceFrame.IMAP_SPACECRAFT | ||
| GSE = SpiceFrame.IMAP_GSE | ||
| GSM = SpiceFrame.IMAP_GSM |
There was a problem hiding this comment.
Do we need to add GSM for the L1D processing as well?
There was a problem hiding this comment.
No, GSM is not used for L1d
Thanks @maxinelasp. I've tested it locally, and it generated all the expected L2 files. Mhairi had a look at them and she was happy. For me this is ready to merge. |
786936a
into
IMAP-Science-Operations-Center:dev
Change Summary
Overview
This change:
a. DSRF transformation not being defined during periods of thruster firing (transformation matrix is now all
NaNs)b. Truncates L2 data to the specified day at the beginning of L2 processing
c. Truncation also crops
epoch_etNote
The current implementation will require a lot of memory for processing Burst mode in 128 cadence (as the data for 1 entire day will be held in memory for 5 different frames).
New Dependencies
N/A
New Files
N/A
Deleted Files
N/A
Updated Files
imap_mag_global_cdf_attrs.yamlmag_l2.py:mag_l2_data.py:epoch_etwhen truncatinggeometry.py:SpiceNOFRAMECONNECT, i.e., no SPICE data is available for a specific timestamp (this happens every day between 10 and 10:30 AM for the despun reference frame, due to thruster firing), aNaNmatrix is returned, instead of an error being throwntest_mag_l2.py:NaNmatrices are supportedtest_geometry.py:get_rotation_matrixreturnsNaNmatrix when despun frame not defined for inputetTesting
Testing was added to cover all cases.