CCSDSPy / ccsdspy

I/O interface and utilities for CCSDS binary spacecraft data in Python. Library used in flight missions at NASA, NOAA, and SWRI
https://ccsdspy.org
BSD 3-Clause "New" or "Revised" License
74 stars 18 forks source link

Implement API for post-processing converters #53

Closed ddasilva closed 1 year ago

ddasilva commented 1 year ago

This change implements part of #51. It adds an API for "converters" to be added to packet fields, which apply post-processing transformations. The established use cases this post-processing are as follows:

  1. Apply calibration curves to digital readout values
  2. Convert Enum-like integers to human-readable strings
  3. Convert binary representations of time to datetime instances

This change develops an API for converters, and implements 1 and 2 of the above. The API is roughly as follows. In addition to using the pre-made converters (currently PolyConverter, LinearConverter, and EnumConverter), users can subclass ccsdspy.converters.Converter and implement a method that applies their desired transformation.

     from ccsdspy import FixedLength, PacketField
     from ccsdspy.converters import LinearConverter, PolyConverter

     pkt = FixedLength([
         ....
         PacketField(
              name="temperature", bit_length=8, data_type='uint',
              converter=LinearConverter(slope=1.2, intercept=-30)
         ),
         PacketField(
              name="current", bit_length=8, data_type='uint',
              converter=PolyConverter(coeffs=[0.1, 1.2, 0.3])
         ),
         ....
     ])

    result = pkt.load('MyCCSDS.bin')   # has conversions applied
ddasilva commented 1 year ago

This change does not implement documentation, that will come later. This will be another page in the user guide that describes the pre-made converters and describes creating your own (for advanced users).

codecov-commenter commented 1 year ago

Codecov Report

Merging #53 (5fe547b) into main (5377bfc) will increase coverage by 0.44%. The diff coverage is 97.39%.

@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
+ Coverage   94.92%   95.37%   +0.44%     
==========================================
  Files           6        7       +1     
  Lines         493      605     +112     
==========================================
+ Hits          468      577     +109     
- Misses         25       28       +3     
Impacted Files Coverage Δ
ccsdspy/packet_fields.py 95.23% <80.00%> (-1.38%) :arrow_down:
ccsdspy/packet_types.py 95.26% <93.54%> (-0.45%) :arrow_down:
ccsdspy/converters.py 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

ddasilva commented 1 year ago

@ehsteve @Hydravine Let me what you think!

ehsteve commented 1 year ago

This is cool! Don't have time to review the code but I do have a few high level comments. It might be good to model this on COSMOS derived items. You can an example of this in this example file. Derived items are always ADDED to the packet. The original field remains which I think is a good idea. They also have a concept of item modifiers which only change how the item is displayed in their GUI. That is probably not relevant here or maybe you'd want to merge the two capabilities like adding units. Astropy provides really amazing units handling and they are numpy arrays. Keeping units next to data is always a good idea.

Anyway, my main feedback is that these conversions should not replace the original field but create a new field with a new name.

WilliamParsonsV commented 1 year ago

Looks excellent! The ability to add custom converters will be extremely useful. Going through the code, my one question is if you might be double-dipping For loops between line [508-514] in packet_types.py and line [51-58] in converters.py as they both seem to be looping on the fields in the field_array?

WilliamParsonsV commented 1 year ago

Also to @ehsteve's comment: This is my first time hearing about derived items but it sounds like a good use for making synthetic parameters obsolete. Currently we create additional ground-based mnemonics for things calculated on the ground (Ex: AVGTEMP that the Flight SW doesn't already do) but instead a derived item could be created and added to the relevant packet object. This might simplify things and keep them tied together/associated if later ingesting into a DB

ddasilva commented 1 year ago

What do you think about the following API designs?

Method 1


pkt = FixedLength([
    ...
])
pkt.add_converted_fields([
     LinearConverter(input_name="input_field1", output_name="converted_input1", slope=1.2, intercept=-30)
     PolyConverter(input_name="input_field2", output_name="converted_input2", coeffs=[0.1, 1.2, 0.3])
])

result = pkt.load('MyCCSDS.bin')   # has both

Method 2


pkt = FixedLength([
    ...
])
pkt.add_converted_field(
    input_name="input_field1", 
    output_name="converted_input1", 
    converter=LinearConverter(slope=1.2, intercept=-30)
)
pkt.add_converted_field(
    input_name="input_field2", 
    output_name="converted_input2", 
    converter=PolyConverter(coeffs=[0.1, 1.2, 0.3])
)

result = pkt.load('MyCCSDS.bin')   # has both

Astropy provides really amazing units handling and they are numpy arrays. Keeping units next to data is always a good idea

I'm on the fence about this. Units are a good idea but including AstroPy as a dependency (which is a very science focused package) might give the wrong message to users who are using this for things completely outside the world of science. I suppose we could accept an optional "units=..." keyword argument to converters and then just return result * units without depending on AstroPy?

ddasilva commented 1 year ago

I went with method 2, and updated the API and wrote a documentation page. Going to add units support and basic time conversion functions next.