Update waveform conversion code to account for new timing fields

[mjohanse-emr]

What does this Pull Request accomplish?

Updates the waveform conversion functions to handle the following new timing fields:

• timestamp
• time_offset
• timestamps

NI-APIs change to add the new fields

Unit tests have been added to test conversion cases specific to these parameters.
Existing unit tests have been updated where necessary (example, don't expect and exception for irregular timing anymore).

Similar change in the C# API

Why should this Pull Request be merged?

To establish parity between our python API and our C# API.
To allow users to publish Irregular Timing waveforms and "Relative Timing" waveforms.

What testing has been done?

Many new tests have been added that were inspired by the C# auto tests.
Existing waveform conversion tests are passing.

[github-actions]

Test Results

  120 files  ±  0    120 suites  ±0   3m 22s ⏱️ ±0s
  291 tests + 40    289 ✅ + 40   2 💤 ±0  0 ❌ ±0 
2 910 runs  +400  2 880 ✅ +400  30 💤 ±0  0 ❌ ±0 

Results for commit aea5175. ± Comparison against base commit 0b84363.

This pull request removes 5 and adds 45 tests. Note that renamed tests count towards both.

tests.unit.test_waveform_conversion ‑ test___analog_waveform_with_irregular_timing___convert___raises_value_error
tests.unit.test_waveform_conversion ‑ test___digital_waveform_round_trip___convert___valid_protobuf
tests.unit.test_waveform_conversion ‑ test___float64_complex_waveform_with_irregular_timing___convert___raises_value_error
tests.unit.test_waveform_conversion ‑ test___int16_analog_waveform_with_irregular_timing___convert___raises_value_error
tests.unit.test_waveform_conversion ‑ test___int16_complex_waveform_with_irregular_timing___convert___raises_value_error

tests.unit.test_waveform_conversion ‑ test___analog_waveform_with_irregular_timing___convert___valid_protobuf
tests.unit.test_waveform_conversion ‑ test___analog_waveform_with_none_timing___round_trip___waveforms_match
tests.unit.test_waveform_conversion ‑ test___analog_waveform_with_standard_timing___round_trip___waveforms_match
tests.unit.test_waveform_conversion ‑ test___analog_waveform_with_standard_timing_and_offset___convert___valid_protobuf
tests.unit.test_waveform_conversion ‑ test___dbl_analog_wfm_with_dt_and_offset___convert___valid_python_object
tests.unit.test_waveform_conversion ‑ test___dbl_analog_wfm_with_incorrect_t0_and_timestamp_and_offset___convert___raises_exception
tests.unit.test_waveform_conversion ‑ test___dbl_analog_wfm_with_t0_and_offset_no_timestamp___convert___raises_exception
tests.unit.test_waveform_conversion ‑ test___dbl_analog_wfm_with_t0_and_timestamp_and_offset___convert___valid_python_object
tests.unit.test_waveform_conversion ‑ test___dbl_analog_wfm_with_timestamps___convert___valid_python_object
tests.unit.test_waveform_conversion ‑ test___dbl_analog_wfm_with_timing___round_trip___waveforms_match
…

♻️ This comment has been updated with latest results.

[mjohanse-emr]

This doesn't exactly have an analog in the C# implementation, but it was already here, so I'm leaving it.

[jasonmreding]

Perhaps this should also be updated to also check that the offset and timestamp fields are also zero or not set? It is valid to set an offset without any timestamp data. Whether setting an offset without also setting dt is perhaps questionable, but I don't know if we should reject it either.

[mjohanse-emr]

This is the meat of the "from_protobuf" conversion methods. Please pay extra attention to this method. It feels a little wonky right now.

[mjohanse-emr]

@jasonmreding @bkeryan
Do we need a round trip test like this one that iterates over different combos of possible parameters and tests against the "normalized" timestamp and start_time?

I'm assuming the python waveforms will also perform the same "normalization" that the C# waveforms do?

[jasonmreding]

The current conversion code in this PR doesn't normalize unset and default timestamps the same way the C# code does. It only checks for field presence of the protobuf message, but it doesn't treat an unset timestamp field as equivalent to a timestamp message set to all default (zero) values. We should probably be consistent in that regard. At that point, having round trip tests like the one you referenced probably makes sense.

I think the C# conversion code doesn't do as much validation and error checking as the conversion code here does, so I'm not sure if all the same test cases will run successfully or not. Later, when we added waveform support to the data store, we also did more error checking/validation then the conversion code added to the APIs package. I would still like to move that validation logic from the data store to the APIs package so it can be reused and optionally start throwing exceptions during the conversion logic. I guess that is something to be mindful of here as well where it might be useful if the waveform validation logic for the protobuf message types were structured in such a way that it can easily be called from other places. For example somebody writing a Python grpc service that accepts waveform data types and they want to ensure the waveform message is populated in a consistent, valid manner.

[mjohanse-emr]

The round_trip tests in python are a little less dynamic than the ones in C#. Is this something that needs to be addressed?

[jasonmreding]

By dynamic, I assume you mean the tests are less data driven and are duplicated per waveform type rather than working from generic types? If so, I think moving the implementation in that direction would be great. However, I'm not going to mandate it. It's also a little above my current expertise with python in guiding you on the best way to accomplish that.

[bkeryan]

I agree that it would be helpful to reduce duplication. I think the most straightforward way to do that is to use @pytest.mark.parametrize to pass in different types of waveform objects or messages. If you can't initialize the input with a single expression, you can call a function or even pass a function as the parameter.

@pytest.mark.parametrize
(
    "waveform",
    [
        AnalogWaveform.from_array_1d(np.array([1.0, 2.0, 3.0])),
        ComplexWaveform.from_array_1d([1.5 + 2.5j, 3.5 + 4.5j], np.complex128),
        ComplexWaveform.from_array_1d([(1, 2), (3, 4)], ComplexInt32DType),
        DigitalWaveform.from_lines(np.array([[0, 1, 0], [1, 0, 1]], dtype=np.bool), signal_count=3),
    ]
)
def test_blah_blah(waveform: AnalogWaveform[Any] | ComplexWaveform[Any] | DigitalWaveform[Any]) -> None:

[bkeryan]

The C# tests are structured using the "test driver" pattern, though they aren't named that way. I think you can implement that in Python by defining a test class with subclasses, rather than test functions.

[mjohanse-emr]

I think I'll try and implement the test class suggestion. Right now, the file is just so huge and there's so much duplication. I'll see where classes get me.

[Copilot]

Pull request overview

Updates the NI waveform <-> protobuf conversion layer to support the newly-added timing fields (timestamp, time_offset, timestamps), and expands unit tests to cover regular/none/irregular timing scenarios and round-trip conversions for multiple waveform types.

Changes:

• Extend waveform-to-protobuf conversion to emit timestamp/time_offset for regular/none timing and timestamps for irregular timing.
• Extend protobuf-to-waveform conversion to interpret timestamps as irregular timing, and to incorporate timestamp + time_offset when building Timing.
• Add/adjust unit tests for the new timing-field behaviors and for round-trip conversions.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 11 comments.

File	Description
packages/ni.protobuf.types/src/ni/protobuf/types/waveform_conversion.py	Implements new timing-field mapping logic in both directions (including irregular timing via timestamps).
packages/ni.protobuf.types/tests/unit/test_waveform_conversion.py	Adds/updates tests for new timing fields and round-trips across waveform types.
.gitignore	Ignores local VS Code settings directory.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jasonmreding]

Clean up this todo. I don't think a conversion to a specific time format is strictly needed. We just need to make sure all three values are in the same time format. I'm not sure if python generics enforce that or if we need to explicitly check for that. We probably also want to apply some fuzziness to the math below.

[mjohanse-emr]

The problem is that I can't do math on these objects in their current types
timestamp: PrecisionTimestamp
t0: PrecisionTimestamp
time_offset: float.

I need to convert them to something that allows me to check that t0 = timestamp + time_offset. I may be mistaken, but converting time_offset to PrecisionTimestamp doesn't work since there are no mathematical operators on PrecisionTimestamp.

[mjohanse-emr]

Or possibly we can just leave this check out. It was suggested by copilot, but that doesn't mean that it's strictly necessary.

However, I think that if both t0 and timestamp are set, offset has to be set to something that satisfies that t0 = timestamp + offset formula.

[jasonmreding]

Is there a reason you changed the numeric constant here? I don't know that it matters much one way or the other, just trying to understand if there was some functional/correctness reason for doing so.

[mjohanse-emr]

Copilot warned me that I wasn't getting sub 1s accuracy because I was using seconds instead of total_seconds somewhere. I was trying to mix up a few of the tests so that they weren't always using 1 second to see if I could catch the failure before switching to total_seconds().

[jasonmreding]

I think we need to fix the logic for the empty waveform conversion. I also think we should be consistent with normalizing unset and default timestamps in the conversion code. If we want to revisit this normalization strategy, I'm fine with that. I mainly just want to make sure the behavior is consistent with the conversions between Python and C#.

[bkeryan]

This discards time_offset. Look at CreateNoIntervalPrecisionTiming in the C# version.

[bkeryan]

What you can do to make this less expensive is to convert all 3 fields into bintime or None up front and pass them into this function so that you don't do any conversions twice.

[bkeryan]

Also, you could convert time_offset to datetime if it's faster, but this component doesn't have any benchmarks for making a decision like this.

[bkeryan]

It would make sense to test all codepaths with non-zero fractional seconds: regular timing, irregular timing, to/from protobuf, etc.

I don't think it's necessary to test all waveform types with non-zero fractional seconds, because they share the same codepaths, but if it's easier to do so, then why not?