[WIP] Initial IDEX L2 implementation #751
Conversation
Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into idex-l2
Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into idex-l2
Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into idex-l2
bourque
added
Ins: IDEX
There was a problem hiding this comment.
This is looking great!
I didn't spend much effort reviewing the actual L2 algorithm itself (I still need to get a better understanding of this myself before I feel like I can comment on it). And this is an initial pass anyways, so we can tackle the algorithm component in a future PR. But I do have a few suggestions for organizing the code.
| @@ -0,0 +1,10 @@ | |||
| """Contains dataclasses to support IDEX processing.""" | |||
There was a problem hiding this comment.
| """Contains dataclasses to support IDEX processing.""" | |
| """Contains constants to support IDEX processing.""" |
| """Contains dataclasses to support IDEX processing.""" | ||
|
|
||
| TARGET_HIGH_CALIBRATION = 0.00135597 | ||
| ION_GRID_CALIBRATION = 0.00026148 |
There was a problem hiding this comment.
I don't see this one being used anywhere in the code. Perhaps that part just isn't implemented yet? Otherwise it could be removed if it is not used.
| time_of_impact_init = 20 | ||
| constant_offset_init = 0.0 | ||
| rise_time_init = 3.71 | ||
| discharge_time_init = 37.1 | ||
| FILLVAL = -1.0e31 | ||
| TOF_Low_Peak_Prominence = 7 |
There was a problem hiding this comment.
| time_of_impact_init = 20 | |
| constant_offset_init = 0.0 | |
| rise_time_init = 3.71 | |
| discharge_time_init = 37.1 | |
| FILLVAL = -1.0e31 | |
| TOF_Low_Peak_Prominence = 7 | |
| TIME_OF_IMPACT_INIT = 20 | |
| CONSTANT_OFFSET_INIT = 0.0 | |
| RISE_TIME_INIT = 3.71 | |
| DISCHARGE_TIME_INIT = 37.1 | |
| FILLVAL = -1.0e31 | |
| TOF_LOW_PEAK_PROMINENCE = 7 |
Suggest to change this in order to be consistently using capitalized variables for constants (which is also suggested by PEP8)
There was a problem hiding this comment.
Without knowing how accurate the values/contents of these are, the structure and organization of the file looks good to me!
| @pytest.fixture() | ||
| def decom_test_data(): | ||
| test_file = Path( | ||
| f"{imap_module_directory}/tests/idex/imap_idex_l0_raw_20230725_v001.pkts" | ||
| ) | ||
| return PacketParser(test_file, "v001").data | ||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def l1_cdf(decom_test_data): | ||
| """ | ||
| from imap_processing.idex.idex_packet_parser import PacketParser | ||
| l0_file = "imap_processing/tests/idex/imap_idex_l0_sci_20230725_v001.pkts" | ||
| l1_data = PacketParser(l0_file, data_version) | ||
| l1_data.write_l1_cdf() | ||
| """ | ||
|
|
||
| return write_cdf(decom_test_data) |
There was a problem hiding this comment.
Since these tests are redundant with what is tested in test_idex_l1.py, I suggest to remove these test functions, save a copy of an IDEX L1 file in the imap_processing/tests/idex/ folder (alongside the test L0 file), and instead write a fixture function here that returns the path to that L1 file. That fixture can then be used by test_l1_xarray() test to process the L1 file.
| return write_cdf(decom_test_data) | ||
|
|
||
|
|
||
| def test_l1_xarray(l1_cdf): |
There was a problem hiding this comment.
This kind of reads to me like its a test that is testing the L1 code, not the L2 code. So I suggest to rename this to something like:
| def test_l1_xarray(l1_cdf): | |
| def test_l2_processing(l1_cdf): |
| l1_data = L2Processor(l1_cdf, "v001").l1_data | ||
| assert l1_data == load_cdf(l1_cdf) |
There was a problem hiding this comment.
| l1_data = L2Processor(l1_cdf, "v001").l1_data | |
| assert l1_data == load_cdf(l1_cdf) | |
| l2_data = L2Processor(l1_cdf, "v001").l1_data | |
| assert l2_data == load_cdf(l1_cdf) |
I think we want to label this as L2 data since it is what is coming out of the L2 pipeline?
| x = impact[impact.attrs["DEPEND_1"]].data | ||
| y = impact.data - np.mean(impact.data[0:10]) |
There was a problem hiding this comment.
I would like to get in the habit of using more descriptive variable names than single characters. I'm not that familiar with the algorithm yet so I don't have a good suggestion right now, but if you can think of a better name for these, that would be welcomed! (Same for the other instances of this in this module)
Change Summary
Overview
Placing idex l2 code into repository and creating l2
.yamlfiles.New Dependencies
lmfit
New Files
Deleted Files
None
Updated Files
None
Testing
Closes #575