-
Notifications
You must be signed in to change notification settings - Fork 16
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.
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
IDEX L1b science data processing #1224
base: dev
Are you sure you want to change the base?
IDEX L1b science data processing #1224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some non-blocking comments. Once you address those, it looks good to me!
|
||
l1b_tof_base: &l1b_tof_base | ||
<<: *l1b_data_base | ||
DEPEND_1: time_high_sr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have a DEPEND_1
, then you will need LABL_PTR_1
instead of LABLAXIS. I tried to capture something about this here, https://imap-processing.readthedocs.io/en/latest/external-tools/cdf/cdf_requirements.html#metadata-data. But let me know if you like to chat about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it! Thanks for linking those docs. I think I need to do this for l1a as well and then can copy them to l1b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good. If this is not blocking this PR, then you can do that in future PR if you like to not make this PR bigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Nice clean, well organized code. I left a few comments to consider.
VAR_TYPE: data | ||
CATDESC: "" | ||
FIELDNAM: "" | ||
FILLVAL: *int_fillval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defined int_fillval
is only valid for 64 bit signed integers. Based on the VALIDMIN
and VALIDMAX
, it seems like a mismatch.
detector_voltage: | ||
<<: *trigger_base | ||
FIELDNAM: Detector Voltage | ||
CATDESC: Last measurement in raw dN for processor board signal - "Detector Voltage" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these CATDESCs say "measurement in raw dN" but the UNITS say Voltage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes those descriptions should be updated thanks
return l1b_dataset | ||
|
||
|
||
def unpack_instrument_settings( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that the fields that get unpacked are not defined in the xtce and unpacked by space_packet_parser
rather than as done here at L1B?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also confused about that. I talked to the IDEX team and they wanted l1a to have all the raw instrument settings so people could go back to l1a and look at them if need be.
l1b_dataset = convert_raw_to_eu( | ||
l1b_dataset, | ||
conversion_table_path=var_information_path, | ||
packet_name=var_information_df["packetName"].to_list(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... The docstring for convert_raw_to_eu
says packet_name
is a str
, not a list
. Is this working correctly? The function reduces the dataframe read in from the conversion table to only the entries where dataframe["packetName"] == packet_name
which I would expect to be an array full of False
, meaning no conversions get applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm it is in fact working as expected. It is converting every value but I am definitely using "packetName" incorrectly. I will have to fix this.
waveforms_pc = {} | ||
|
||
for var in ConvFactors: | ||
waveforms_pc[var.name] = l1a_dataset[var.name] * var.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that numpy will convert the dtype to a float
in this statement. Is that the desired dtype for the output CDF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, you are making great progress! I have a few minor suggestions if you want to use them, otherwise this looks good.
Change Summary
IDEX l1b science data processing and tests
Overview
Adds processing to unpack telemetry items using imap_processing/idex/idex_variable_unpacking_and_eu_conversion.csv, and converts all instrument setting variables and waveforms to engineering units.
Addition of spice data and two more attributes will be added in another PR.
closes #833
New Files
Updated Files
Testing