Skip to content

Fix ADC parameter calculation overflow errors - #553

Merged
tompollard merged 7 commits into
mainfrom
bg_fix_overflow
Nov 4, 2026
Merged

tompollard merged 7 commits into
mainfrom
bg_fix_overflow

Conversation

@briangow

@briangow briangow commented Jan 7, 2026

Copy link
Copy Markdown
Contributor

As discussed in #515, an overflow can occur when calculating ADC parameters from limited-precision physical signal data. The root cause of this overflow is precision loss. When computing min/max values during parameter calculation, insufficient floating-point precision at boundaries (e.g. int32 boundaries) leads to gain/baseline values that cause digital samples to overflow by exactly 1 value.

The fix ensures float64 precision is used throughout ADC parameter calculation and conversion:

  • In calc_adc_params(): Convert physical signals to float64 before computing min/max
  • In adc_inplace(): Convert to float64 before performing gain/baseline arithmetic

Users can now safely provide lower-precision physical data (e.g., float32) without validation errors.

This PR includes a test for the overflow which uses the data supplied by @Ivorforce in #515. The test checks that:

  • float32 input data can be written with fmt=32 without overflow
  • digital values stay within the valid range
  • the physical signal round-trips accurately through write/read

@briangow
briangow requested a review from bemoody February 3, 2026 16:21
@tompollard

Copy link
Copy Markdown
Member

Thanks Brian. I checked this against current main: the CSV reproduces the overflow (IndexError on channel 1), the fix resolves it, and both the min/max and adc_inplace changes are needed.

A few requests:

  • Could you update the branch from main? It predates Fix ADC gain formula #551 and the recent pandas changes, so import wfdb currently fails on the branch with pandas 3.

  • In calc_adc_params, we don't need to copy the full signal to float64 just to get min/max. Converting float32 to float64 doesn't change the values; what matters is that minvals/maxvals are float64, so the gain/baseline arithmetic runs in float64. Something like this gives the same result without the extra copy:

    minvals = np.nanmin(self.p_signal, axis=0).astype("float64")
    maxvals = np.nanmax(self.p_signal, axis=0).astype("float64")
    # expanded path:
    minvals.append(float(np.nanmin(ch)))
    maxvals.append(float(np.nanmax(ch)))
    
    It'd be good to update the comments to say this.
  • In the non-inplace path of adc(), .copy() followed by astype("float64") now copies twice for float32 input. Passing .astype("float64") instead of .copy() would avoid that.

@tompollard tompollard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Brian, looks great. Before we merge, could you add a changelog fragment, e.g. changelog.d/adc-float32-overflow.rst? Something like: "Fixed wrsamp failing with an out-of-range error when writing float32 physical signals, by computing ADC parameters in float64."

@briangow

briangow commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Added the changelog @tompollard , thanks.

@tompollard

Copy link
Copy Markdown
Member

Looks good, thanks!

@tompollard
tompollard merged commit d8c1ef4 into main Nov 4, 2026
18 checks passed
@tompollard
tompollard deleted the bg_fix_overflow branch November 4, 2026 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants