-
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
I-ALiRT: L0 parsing #246
I-ALiRT: L0 parsing #246
Conversation
I intend to add more tests once I have test data. Just FYI. |
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.
Overall I think this looks good. One thing that would help is adding an example of what the xarray dataset should look like. Something like printing ds.head()
and putting that into a comment in the code so that it is easy for someone not as familiar with this to quickly grasp how you expect things to look.
class PacketLength(Enum): | ||
"""Class that represents properties of the IALiRT packet.""" | ||
|
||
EXPECTED_LENGTH = 1464 |
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.
Do we need an Enum for this, or just call it IALIRT_PACKET_LENGTH = 1464
as a global variable?
return Path(sys.modules[__name__.split( | ||
'.')[0]].__file__).parent / 'ialirt' / 'tests' / 'test_data' / 'l0' / \ | ||
'IALiRT Raw Packet Telemetry.txt' |
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.
Do we need to look at the module name, or can we just look at the file path itself? Also below.
return Path(sys.modules[__name__.split( | |
'.')[0]].__file__).parent / 'ialirt' / 'tests' / 'test_data' / 'l0' / \ | |
'IALiRT Raw Packet Telemetry.txt' | |
return Path(__file__).parent.parent / 'test_data' / 'l0' / \ | |
'IALiRT Raw Packet Telemetry.txt' |
Or you could use the variable that comes with the package:
imap_module_directory = Path(__file__).parent |
@pytest.fixture(scope="session") | ||
def decom_data(packet_path, xtce_ialirt_path): | ||
"""Data for decom_ultra""" | ||
data_packet_list = generate_xarray(packet_path, xtce_ialirt_path) | ||
return data_packet_list | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def decom_test_data(packet_path, xtce_ialirt_path): | ||
"""Read test data from file""" | ||
data_packet_list = decom_packets(packet_path, xtce_ialirt_path) | ||
return data_packet_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.
These two seem really similar to me and I didn't quite know the different due to the variable namings both being list at first glance.
I think if you move the decom_packets
code you wrote into a test function here instead, then you could use imap_processing.decom.decom_packets()
to do your packet list creation inside of generate_xarray()
and not need both of these fixtures.
|
||
|
||
def test_decom_instruments(decom_data, decom_test_data): | ||
"""This function checks that all instrument parameters are accounted for.""" |
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'm not sure I fully follow all of this test. I think it'd help to break this down even further and actually assert on individual instruments, possibly parameterizing this over instruments if that would help.
But, I think it'd be good to say some things about each instrument individually and spell it out explicitly here. assert len(decom_data["HIT"]["data"]) == expected HIT data size
or, I don't think it would be unreasonable to also make an xarray dataset manually with np.zeros(proper_shape)
and set the values you expect here so that you are asserting against a known zero array of the proper shape without having to decipher the loops within the test.
<xtce:Parameter name="MAG_ACQ" parameterTypeRef="uint48"> | ||
<xtce:LongDescription>MAG Acquisition Time</xtce:LongDescription> | ||
</xtce:Parameter> | ||
<xtce:Parameter name="MAG_STATUS" parameterTypeRef="uint24"> | ||
<xtce:LongDescription>MAG Status</xtce:LongDescription> | ||
</xtce:Parameter> | ||
<xtce:Parameter name="MAG_DATA" parameterTypeRef="uint24"> | ||
<xtce:LongDescription>MAG Data</xtce:LongDescription> | ||
</xtce:Parameter> |
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.
There are more bits in the MAG Acquisition Time than the data?
I think the data fields might need to be BinaryParameterType / BYTE so you can unpack the bits once you get more info later.
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.
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! That just seems odd to me, but you're definitely right. One thing I did just notice though is that it looks like the I-ALiRT spreadsheets have MET coming down, which is 32 bits instead of ACQ_TM which is 48 bits. Is there another mismatch there that we need to update in the I-ALiRT spreadsheet as well?
@maxinelasp did you have any conversations with MAG about their I-ALiRT data formatting?
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.
Yes. I had to piece together different spreadsheets to get the xml:
- I-ALiRT Packet Definitions 2022_10_25.xlsx (old)
- IALiRT SC Packet Content.xlsx (recent)
- And then what was in IMAPCATS
So you are correct. The definitions from I-ALiRT Packet Definitions 2022_10_25.xlsx need to be updated.
I am going to look at this once more on Monday. So please don't review again yet. |
pre-commit.ci autofix |
with tempfile.NamedTemporaryFile(delete=False) as tmp_file: | ||
binary_file_path = Path(tmp_file.name) |
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.
We want to delete the temporary file on clean-up, so we don't want this to be delete=False
.
https://docs.pytest.org/en/7.1.x/how-to/tmp_path.html
I think you can get what you are after here with the tmp_path
fixture from pytest directly, and it will be cleaned up for you after the yield is finished. (meaning you don't need the unlink later either.
@pytest.fixture(scope="session") | ||
def empty_binary_packet_path(): | ||
""" | ||
Creates an empty binary file, representative of a scenario | ||
in which no packet data is available. | ||
""" | ||
with tempfile.NamedTemporaryFile(delete=False) as tmp_file: | ||
binary_file_path = Path(tmp_file.name) | ||
|
||
yield binary_file_path | ||
|
||
binary_file_path.unlink() |
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'd remove this entire block and put the relevant tmp_path usage inside the test below since you're only using it once it isn't really a fixture.
@pytest.fixture(scope="session") | |
def empty_binary_packet_path(): | |
""" | |
Creates an empty binary file, representative of a scenario | |
in which no packet data is available. | |
""" | |
with tempfile.NamedTemporaryFile(delete=False) as tmp_file: | |
binary_file_path = Path(tmp_file.name) | |
yield binary_file_path | |
binary_file_path.unlink() |
Validates the length of binary data converted | ||
from a text file containing hexadecimal packet data. |
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.
This function is sort of recreating what you're already doing in the fixture above. You could just add the assert up there if you're trying to test that file.
def test_generate_xarray_empty_file(empty_binary_packet_path, xtce_ialirt_path, caplog): | ||
"""Test that an error is logged if an empty file is passed to generate_xarray.""" | ||
|
||
result = generate_xarray(empty_binary_packet_path, xtce_ialirt_path) | ||
|
||
assert "Error during packet decomposition" in caplog.text | ||
assert result is None |
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.
def test_generate_xarray_empty_file(empty_binary_packet_path, xtce_ialirt_path, caplog): | |
"""Test that an error is logged if an empty file is passed to generate_xarray.""" | |
result = generate_xarray(empty_binary_packet_path, xtce_ialirt_path) | |
assert "Error during packet decomposition" in caplog.text | |
assert result is None | |
def test_generate_xarray_empty_file(empty_binary_packet_path, xtce_ialirt_path, caplog): | |
"""Test that an error is logged if an empty file is passed to generate_xarray.""" | |
with pytest.raises(ValueError): | |
generate_xarray(empty_binary_packet_path, xtce_ialirt_path) |
If you raise instead of log, we can assert that this does raise as expected here like this.
# Check that all the dimensions are the correct length | ||
# example:len(xarray_data['HIT'].coords['HIT_SC_TICK'].values) == 32 | ||
for instrument in instruments: | ||
dimension_name = next(iter(xarray_data[instrument].dims)) |
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.
Do you need to get all dimension names? Right now I think you're just getting the first one.
dimension_name = next(iter(xarray_data[instrument].dims)) | |
for dimension_name in xarray_data[instrument].dims: |
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.
Nice! Looks great.
yield binary_file_path | ||
|
||
return binary_file_path |
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.
Do we need both of these? I'm not sure which one would be needed to make sure the tmp_path
doesn't get cleaned up too soon.
ec06ec1
into
IMAP-Science-Operations-Center:dev
* adding ialirt
* adding ialirt
* update telemetry_generator to be more generic * Updates to telemetry generator * updating docs * updating to remove local-specific testing changes * first pass at glows xtce * update way of checking if description exists or not. update generator template. * updated generator to use correct IntegerParameterType * First pass at GLOWS decom * updating telemetry generator * Updating glows decom to properly process data into dictionary * Finished histogram decom * Updating decom to include direct events * Finalizing code, adding smaller test data file and combined XML, updating documentation and tests * Updating test file path * Fixing test runs * updating docs * Updating data classes to use attributes, and changing names * Removing keys from data class and getting attribute names from the packet * Move INT to SINT in telemetry_generator Co-authored-by: Tenzin Choedon <36522642+tech3371@users.noreply.github.com> * Addressing code review comments * Fixing test failure * Review updates * fixing typo * Updating IntEnum to Enum to fix automatic documentation * Move L0 data classes to a dataclass type with specific attributes * Initial GLOWS L1A data classes * Adding required attributes to L0 data class * Adding L1A histogram data class * fixing test failures * Finishing ccsds dataclass * Minor updates * Finishing GLOWS L1A histograms * Updating to include direct events data * Updating glows l1a * Adding useful GLOWS code from GLOWS team and Marek Strumik * Adding direct event information Co-authored-by: GLOWS team <maro@cbk.waw.pl> * Updates for direct event creation * Updates to L1A direct events * Updated release workflow to reflect removal of "main" branch and use of version branches * Reduced the filesize of git-workflow.png * Glows L0 changes to data class to accomodate L1A needs (#264) * Glows L0 changes to data class to accomodate L1A needs * DOC: Add intersphinx to documentation for external references * DOC: Add a clean option to the documentation Makefile build command * DOC: Update glows types in dataclasses * DOC: Change glows currentmodule references * DOC: Add ccsds header to glows documentation * DOC: Add inherited member description for glows This adds the inherited attributes from the dataclass directly into the subclasses. There may be a better way to do this, so perhaps some setting/configuration parameter needs to be updated? But this at least fixes the warnings for now. * FIX: Run black on glows test * PR updates --------- Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com> * Housekeeping data testing and validation for CoDICE l0 (#176) Validated housekeeping data with tests plus minor updates --------- Co-authored-by: Matthew Bourque <matthew.bourque@lasp.colorado.edu> * L1 cdf creation (#179) * Extracting and adding new data to the IDEX L1 * Adding some of the values to the XML, rather than shifting bits in the code (work in progress, need to do a few more variables) * Finishing up having space packet parser decom * Getting rid of unecessary bit masksi8kt9gloh * making changes based on comments * Formatting the file after the upstream sync * Adding L1 CDF creation * Forgot to commit poetry changes * Making changes based on Greg's comments, as well as issues 1-8 from SPDF * Fixing import statements * Changing version into a string * Adding more descriptions, units, and labels in the attributes * Fixing the way strings are inserted into the CDF * Fixing issues with white spice in the text of attrs * Files are finally ISTP compliant! * Fixing a couple things for ISTP * Had the contents of the 32 bit things reversed * Fixing the packet definition for the CCSDS file * More fixes to ensure ISTP compliance * Updating the file name * touching finishes on the L1 CDF * Adding fixture for temp directory in tests * Fixing one of the ruff errors by adding a namedtuple * Adding new "write_cdf" function that writed cdfs based on attributes * Getting rid of a few more magic numbers * Updating the packet definition * initial hit l1a decom (#164) * initial hit l1a decom * using defaultdict, add docs * fixed hit l1a docstrings * fixed hit l1a docstring formatting * doc updates and minor fixes * minor docstring update * minor comment update * rebased with dev * fixed ruff issues * regenerated xtce * fixed unit test * github doc gen fix attempt * added class docstring attributes * fixed more doc issues * another doc fix attempt * doc fix attempt * doc fix attempt * changed doc python v to 3.10 * I-ALiRT: L0 parsing (#246) * adding ialirt * CDF documentation update (#273) * CDF documentation update * Adding the cdf_guide.rst to the toc * Formatting update! * Updating the docs for sections I missed * Formatting a little nicer * Adding links enabled in the docs * Fixing duplicate headers * Getting rid of duplicate headers * Fixing more documentation * Responding to a few code review comments * Breaking up the page into sub-pages * Adding a link to xarray_to_cdf page * Adding more links to different sections * fixing image link * Editting some of the wording now that we're on different pages * Typos and format fixed * Another format error * Create CDF class based system (#272) * Updating IDEX CDF attrs * remove comments, create constants class * Updating names and addressing PR comments * Updating docs * Adding tests * Updating types of L0 to properly read bytearrays * Updating tests * GLOWS direct events compressed and uncompressed * updating decom dataclasses * Updating GLOWS L0 * L1A tests * removing unneeded code * updating tests and fixing minor bug * fixing autodocs * addressing comments from code review * Updating constants to a frozen dataclass * Updating direct events packet reading to correctly read packet data, updating test data, adding tests * Updating docstrings --------- Co-authored-by: Tenzin Choedon <tenzin.choedon@lasp.colorado.edu> Co-authored-by: Tenzin Choedon <36522642+tech3371@users.noreply.github.com> Co-authored-by: GLOWS team <maro@cbk.waw.pl> Co-authored-by: Matthew Bourque <matthew.bourque@lasp.colorado.edu> Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com> Co-authored-by: Gabriel M <104743000+GFMoraga@users.noreply.github.com> Co-authored-by: Bryan Harter <41062454+bryan-harter@users.noreply.github.com> Co-authored-by: Sean Hoyt <sean.hoyt@lasp.colorado.edu> Co-authored-by: Laura Sandoval <46567335+laspsandoval@users.noreply.github.com>
* update telemetry_generator to be more generic * Updates to telemetry generator * updating docs * updating to remove local-specific testing changes * first pass at glows xtce * update way of checking if description exists or not. update generator template. * updated generator to use correct IntegerParameterType * First pass at GLOWS decom * updating telemetry generator * Updating glows decom to properly process data into dictionary * Finished histogram decom * Updating decom to include direct events * Finalizing code, adding smaller test data file and combined XML, updating documentation and tests * Updating test file path * Fixing test runs * updating docs * Updating data classes to use attributes, and changing names * Removing keys from data class and getting attribute names from the packet * Move INT to SINT in telemetry_generator Co-authored-by: Tenzin Choedon <36522642+tech3371@users.noreply.github.com> * Addressing code review comments * Fixing test failure * Review updates * fixing typo * Updating IntEnum to Enum to fix automatic documentation * Move L0 data classes to a dataclass type with specific attributes * Initial GLOWS L1A data classes * Adding required attributes to L0 data class * Adding L1A histogram data class * fixing test failures * Finishing ccsds dataclass * Minor updates * Finishing GLOWS L1A histograms * Updating to include direct events data * Updating glows l1a * Adding useful GLOWS code from GLOWS team and Marek Strumik * Adding direct event information Co-authored-by: GLOWS team <maro@cbk.waw.pl> * Updates for direct event creation * Updates to L1A direct events * Updated release workflow to reflect removal of "main" branch and use of version branches * Reduced the filesize of git-workflow.png * Glows L0 changes to data class to accomodate L1A needs (IMAP-Science-Operations-Center#264) * Glows L0 changes to data class to accomodate L1A needs * DOC: Add intersphinx to documentation for external references * DOC: Add a clean option to the documentation Makefile build command * DOC: Update glows types in dataclasses * DOC: Change glows currentmodule references * DOC: Add ccsds header to glows documentation * DOC: Add inherited member description for glows This adds the inherited attributes from the dataclass directly into the subclasses. There may be a better way to do this, so perhaps some setting/configuration parameter needs to be updated? But this at least fixes the warnings for now. * FIX: Run black on glows test * PR updates --------- Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com> * Housekeeping data testing and validation for CoDICE l0 (IMAP-Science-Operations-Center#176) Validated housekeeping data with tests plus minor updates --------- Co-authored-by: Matthew Bourque <matthew.bourque@lasp.colorado.edu> * L1 cdf creation (IMAP-Science-Operations-Center#179) * Extracting and adding new data to the IDEX L1 * Adding some of the values to the XML, rather than shifting bits in the code (work in progress, need to do a few more variables) * Finishing up having space packet parser decom * Getting rid of unecessary bit masksi8kt9gloh * making changes based on comments * Formatting the file after the upstream sync * Adding L1 CDF creation * Forgot to commit poetry changes * Making changes based on Greg's comments, as well as issues 1-8 from SPDF * Fixing import statements * Changing version into a string * Adding more descriptions, units, and labels in the attributes * Fixing the way strings are inserted into the CDF * Fixing issues with white spice in the text of attrs * Files are finally ISTP compliant! * Fixing a couple things for ISTP * Had the contents of the 32 bit things reversed * Fixing the packet definition for the CCSDS file * More fixes to ensure ISTP compliance * Updating the file name * touching finishes on the L1 CDF * Adding fixture for temp directory in tests * Fixing one of the ruff errors by adding a namedtuple * Adding new "write_cdf" function that writed cdfs based on attributes * Getting rid of a few more magic numbers * Updating the packet definition * initial hit l1a decom (IMAP-Science-Operations-Center#164) * initial hit l1a decom * using defaultdict, add docs * fixed hit l1a docstrings * fixed hit l1a docstring formatting * doc updates and minor fixes * minor docstring update * minor comment update * rebased with dev * fixed ruff issues * regenerated xtce * fixed unit test * github doc gen fix attempt * added class docstring attributes * fixed more doc issues * another doc fix attempt * doc fix attempt * doc fix attempt * changed doc python v to 3.10 * I-ALiRT: L0 parsing (IMAP-Science-Operations-Center#246) * adding ialirt * CDF documentation update (IMAP-Science-Operations-Center#273) * CDF documentation update * Adding the cdf_guide.rst to the toc * Formatting update! * Updating the docs for sections I missed * Formatting a little nicer * Adding links enabled in the docs * Fixing duplicate headers * Getting rid of duplicate headers * Fixing more documentation * Responding to a few code review comments * Breaking up the page into sub-pages * Adding a link to xarray_to_cdf page * Adding more links to different sections * fixing image link * Editting some of the wording now that we're on different pages * Typos and format fixed * Another format error * Create CDF class based system (IMAP-Science-Operations-Center#272) * Updating IDEX CDF attrs * remove comments, create constants class * Updating names and addressing PR comments * Updating docs * Adding tests * Updating types of L0 to properly read bytearrays * Updating tests * GLOWS direct events compressed and uncompressed * updating decom dataclasses * Updating GLOWS L0 * L1A tests * removing unneeded code * updating tests and fixing minor bug * fixing autodocs * addressing comments from code review * Updating constants to a frozen dataclass * Updating direct events packet reading to correctly read packet data, updating test data, adding tests * Updating docstrings --------- Co-authored-by: Tenzin Choedon <tenzin.choedon@lasp.colorado.edu> Co-authored-by: Tenzin Choedon <36522642+tech3371@users.noreply.github.com> Co-authored-by: GLOWS team <maro@cbk.waw.pl> Co-authored-by: Matthew Bourque <matthew.bourque@lasp.colorado.edu> Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com> Co-authored-by: Gabriel M <104743000+GFMoraga@users.noreply.github.com> Co-authored-by: Bryan Harter <41062454+bryan-harter@users.noreply.github.com> Co-authored-by: Sean Hoyt <sean.hoyt@lasp.colorado.edu> Co-authored-by: Laura Sandoval <46567335+laspsandoval@users.noreply.github.com>
Change Summary
Overview
Decom I-ALiRT packet data and create xarray.
New Dependencies
None
New Files
Testing
Note:
No test data was provided so far, so further tests will be added at a later date.