Initial delivery of NEMISIS level 1 processing software #26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…te a L1 file, and updated test procedure
…ation.py, using CCSDSpy
* Raises a ValueError if a zero-length file is encountered, rather than an obsure CCSDSpy error about the 6 byte header length. * Calculates checksums using ccsdspy.utils.split_packet_bytes to retrieve the packet bytes. * Z-version of output file is taken from the version of the Level 0 input file. * calculates the packet time for initial packets with special operations status: RELAY_STATE == 0 OR START_FLAG == 1 OR XSUM == 0. If the file concludes with this type of packet, then it gets tossed. * pre-allocates data arrays, rather than concatenating sample by sample… * decodes the RT clock, including Binary Coded Decimal conversion, and converting a GPS time for storage. * stores all packet status data (packet id, acqisition position number, etc.) in the CDF.
…ata_directory can be used.
Created a ‘sampflags’ parameter to flag individual 10 sample/second samples, such as the first data point in a start-up packet, or all data points in the packet when the checksum is bad. Added MET_SECONDS, SCTF_SECONDS and SCTF_SUBSECONDS quantities to the Level 1 output. Corrected the data type for PKT_ID and ACQ_POS_NUM in the Level 1 CDF.
Alrobbertz
reviewed
Aug 21, 2024
| file_metadata["time"], | ||
| "l1", | ||
| f'1.0.{file_metadata["version"]}', | ||
| cdf_version = f'1.0.{int(file_metadata["version"])}' |
| ) | ||
| log.info(f"Finished Creating HermesData structure") | ||
|
|
||
| cdf_file_path = nemisis_data.save(output_path=hermes_nemisis._data_directory, overwrite=True) |
There was a problem hiding this comment.
@dbarrous is this the correct directory to save to the S3 buckets?
…re used to calculate and record the checksum. Added comments to describe the Z-version convention for the Level 1 output filename. readbin_CCSDS.py: file deleted because it is obsolte. test_calibration.py: * removed test code associated with the obsolete readbin_CCSDS.py. * added more comments regarding the useful, but large, test file that is not included in the repository
|
Hi Andrew,
Please see my responses to each of your comments in line, below.
I have pushed up the corresponding changes to my ccsdspy branch on GitHub
-Ken
From: Andrew Robbertz ***@***.***>
Date: Wednesday, August 21, 2024 at 9:31 AM
To: HERMES-SOC/hermes_nemisis ***@***.***>
Cc: Bromund, Kenneth R. (GSFC-5860) ***@***.***>, Author ***@***.***>
Subject: [EXTERNAL] [BULK] Re: [HERMES-SOC/hermes_nemisis] Initial delivery of NEMISIS level 1 processing software (PR #26)
CAUTION: This email originated from outside of NASA. Please take care when clicking links or opening attachments. Use the "Report Message" button to report suspicious messages to the NASA SOC.
@Alrobbertz commented on this pull request.
________________________________
In hermes_nemisis/calibration/calibration.py<#26 (comment)>:
data = pkt.load(data_filename)
+
+ # calculate checksum of each packet
+ n_pkts = len(data['CHECKSUM'])
+ packet_bytes = ccsdspy.utils.split_packet_bytes(
+ data_filename,
+ include_primary_header=False
+ )
+ calculated_checksum_out = np.zeros(n_pkts, dtype=np.uint16)
+ for i in range(n_pkts):
+ CK_A = 0
+ CK_B = 0
+ for octet_num in range(1115):
Is this always going to be the same? Is this 1115 a constant or is it potentially variable? If its a constant, maybe ad a comment on where it comes from.
Yes, it will always be the same. I added a comment.
________________________________
In hermes_nemisis/calibration/calibration.py<#26 (comment)>:
file_metadata = parse_science_filename(original_filename.name)
- cdf_filename = original_filename.parent / create_science_filename(
- file_metadata["instrument"],
- file_metadata["time"],
- "l1",
- f'1.0.{file_metadata["version"]}',
+ cdf_version = f'1.0.{int(file_metadata["version"])}'
I believe all the files should already contain an X.Y.Z version. I know the test file in this PR does not match this, but will the files we receive from the MOC have the same versioning scheme @dbarrous<https://github.com/dbarrous> @ehsteve<https://github.com/ehsteve> ?
In the example L0 files, there is a single, two-digit version number. I assume that this convention is defined by the MOC, and I think it is fine. I am adopting a convention where the Z-version of the Level 1 file will reflect the version of the Level 0 file, but without the leading zero (hence the conversion to int before formatting as a string in my code). I have added a comment to the code to explain this.
We discussed that the instrument data processing code should be responsible for defining the X.Y.Z version for its output files. The X-version number will be hard-coded, as it is a function of the instrument software version. The Y-version number will be taken from the calibration file, but this doesn’t apply to Level 1 files, so here, the Y version is hard-coded as 0. I hope that we can adopt HERMES SOC-wide guidelines for the X.Y.Z version number that are compatible with my intended scheme, but I am open to further discussion. Note that in this scheme, the X and Y versions are not relevant to Level 0 files: the X.Y.Z scheme need not apply to MOC-generated files.
________________________________
In hermes_nemisis/calibration/calibration.py<#26 (comment)>:
- return cdf_filename
+ nemisis_data.timeseries["Epoch_pkt"]["hermes_nem_checksum_valid"].meta.update(
+ OrderedDict({"CATDESC":"telemetry packet checksum valid"})
+ )
+ nemisis_data.timeseries["Epoch_pkt"]["hermes_nem_checksum"].meta.update(
+ OrderedDict({"CATDESC":"telemetry packet checksum"})
+ )
+ nemisis_data.timeseries["Epoch_pkt"]["hermes_nem_calculated_checksum"].meta.update(
+ OrderedDict({"CATDESC":"telemetry packet calculated checksum"})
+ )
+ log.info(f"Finished Creating HermesData structure")
+
+ cdf_file_path = nemisis_data.save(output_path=hermes_nemisis._data_directory, overwrite=True)
@dbarrous<https://github.com/dbarrous> is this the correct directory to save to the S3 buckets?
Let me know the correct method, and I will adjust the code, as necessary.
________________________________
On hermes_nemisis/calibration/readbin_CCSDS.py<#26 (comment)>:
It doesn't look like this file or function is used in the calibration.py workflows and it looks like the unit tests for this file are commented out? Is this really needed as a part of this PR? It looks like this is more of a test script and the contents of this function are broken up among other functions in the calibration.py file.
Deleted.
________________________________
In hermes_nemisis/tests/test_calibration.py<#26 (comment)>:
+# def test_readbin_CCSDS():
+# output_file = readbin_CCSDS('hermes_nemisis/data/hermes_NEM_l0_2024085-174022_v00.bin')
+# # assert output_file.is_file() # this fails. output_file is a string object, not a file. is this a HermesData issue?
If the readbin_CCSDS.py file is not needed, can you remove these commented out test cases? If it is needed, then you should un-comment the test case and validate that they pass.
Done. I also added some clarifying comments regarding the commented-out test file that is useful to us, but not included in the repo.
—
Reply to this email directly, view it on GitHub<#26 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BBMIDVKGJB63K7SCQURKEK3ZSSJDJAVCNFSM6AAAAABMM6TCBCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENJQG44DMNJSGQ>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Hi @kbromund, thank you for addressing many of the comments here. It does not look like the changes you mentioned were successfully pushed to this branch. Could you try to make sure the changes are committed and pushed up? If you'd like help or a second set of eyes, please send me an email or a message on slack and I'd be happy to take a look. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HERMES/NEMISIS Level 1 processing software and sample Level 0 data.
pytest will successfully process Level 0 to Level 1.
This branch was named ccsdspy to distinguish it from an earlier version that did not use ccsdspy.