Skip to content

fix: give points with invalid uncertainties no weight instead of dropping all uncertainties or making the residual infinite - #242

Merged
sbillinge merged 3 commits into
diffpy:mainfrom
cadenmyers13:uncertainty-checks
Oct 31, 2026
Merged

sbillinge merged 3 commits into
diffpy:mainfrom
cadenmyers13:uncertainty-checks

Conversation

@cadenmyers13

@cadenmyers13 cadenmyers13 commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR address?

A point whose uncertainty is zero, negative, infinite or NaN now gets no weight in a fit. Before, ProfileParser dropped the whole dy column and weighted every point equally, and Profile.set_observed_profile gave an infinite residual. If no point in the fit range has a valid uncertainty, the fit is weighted equally, as for data with no uncertainties. FitResults says how many points have no weight and leaves them out of Chi2, Reduced Chi2 and Rw.

Case 1: some uncertainties are valid

# x      y        dy
  0.0    0.012    0.05
  1.0    0.061    0.05
  2.0    0.398    0.05
  3.0    1.104    0.05
  4.0    2.231    0.05
  5.0    3.017    0.05
  6.0    2.204    0.05
  7.0    1.096    0.05
  8.0    0.405    0.00 <-- invalid
  9.0    0.057    0.00 <-- invalid

Before:

  • ProfileParser dropped the whole dy column because of the two zeros, so all 10 points were weighted equally, without a warning.
  • set_observed_profile kept the zeros, so the residual was infinite at those points and least_squares stopped with ValueError: Residuals are not finite in the initial point.

After: The two points with σ = 0 get no weight. The other eight are fitted with their σ, and the results say how many points have no weight.

2 of the 10 points of contribution 'peak' have no weight, because their uncertainties are zero, negative, infinite or not a number. To include them in the fit, please set valid uncertainties with Profile.set_observed_profile.
Overall
------------------------------------------------------------------------------
Residual       9.45555279
Contributions  9.45555279
Restraints     0.00000000
Chi2           9.45555279
Reduced Chi2   1.89111056
Rw             0.03314337

Variables
------------------------------------------------------------------------------
A   2.92662214e+00 +/- 3.92960982e-02
w   2.01165799e+00 +/- 3.34955937e-02
x0  4.98473152e+00 +/- 2.22086687e-02

Case 2: no uncertainty is valid

# x      y        dy <-- all invalid
  0.0    0.012    0.00
  1.0    0.061    0.00
  2.0    0.398    0.00
  3.0    1.104    0.00
  4.0    2.231    0.00
  5.0    3.017    0.00
  6.0    2.204    0.00
  7.0    1.096    0.00
  8.0    0.405    0.00
  9.0    0.057    0.00

Before:

  • ProfileParser dropped the dy column, so the points were weighted equally.
  • set_observed_profile kept the zeros, so the residual was infinite everywhere and least_squares stopped with ValueError: Residuals are not finite in the initial point.

After: Data loaded with ProfileParser behave as before. Data set with set_observed_profile now fit with equal weights instead of stopping with an error, the same as for data with no dy column, and the existing note says the uncertainties are missing.

Some quantities invalid due to missing profile uncertainty
Overall (Chi2 and Reduced Chi2 invalid)
------------------------------------------------------------------------------
Residual       0.03094618
Contributions  0.03094618
Restraints     0.00000000
Chi2           0.03094618
Reduced Chi2   0.00442088
Rw             0.03777508

Variables (Uncertainties invalid)
------------------------------------------------------------------------------
A   2.91352045e+00 +/- 7.66851505e-01
w   2.03554644e+00 +/- 6.18781296e-01
x0  4.99318985e+00 +/- 4.37425518e-01

Case 3: all uncertainties in the fit range are valid

The data are the same as case 1, with the points with σ = 0 cut off:

# x      y        dy
  0.0    0.012    0.05
  1.0    0.061    0.05
  2.0    0.398    0.05
  3.0    1.104    0.05
  4.0    2.231    0.05
  5.0    3.017    0.05
  6.0    2.204    0.05
  7.0    1.096    0.05   <-- xmax
  8.0    0.405    0.00
  9.0    0.057    0.00
profile.set_calculation_range(xmax=7.0)

Before:

  • ProfileParser had already dropped the whole dy column, so the eight points in the range were weighted equally even though all their σ were valid.
  • set_observed_profile gave a normal weighted fit.

After: Data loaded with ProfileParser now give a normal weighted fit. Data set with set_observed_profile behave as before. The results are identical to case 1, because points with no weight count as if they weren't in the data.

Overall
------------------------------------------------------------------------------
Residual       9.45555279
Contributions  9.45555279
Restraints     0.00000000
Chi2           9.45555279
Reduced Chi2   1.89111056
Rw             0.03314337

Variables
------------------------------------------------------------------------------
A   2.92662214e+00 +/- 3.92960982e-02
w   2.01165799e+00 +/- 3.34955937e-02
x0  4.98473152e+00 +/- 2.22086687e-02

Case 4: no uncertainty in the fit range is valid

The data are the same as case 1, but the fit range covers only the points with σ = 0:

# x      y        dy
  0.0    0.012    0.05
  1.0    0.061    0.05
  2.0    0.398    0.05
  3.0    1.104    0.05
  4.0    2.231    0.05
  5.0    3.017    0.05
  6.0    2.204    0.05
  7.0    1.096    0.05
  8.0    0.405    0.00   <-- xmin
  9.0    0.057    0.00
profile.set_calculation_range(xmin=8.0)

Before:

  • ProfileParser had dropped the dy column, so the points were weighted equally.
  • set_observed_profile kept the zeros, so least_squares stopped with ValueError: Residuals are not finite in the initial point.

After: Behaves as case 2. Only the uncertainties inside the fit range count, so the points are weighted equally (profile.dy is [1, 1]), and the results say the uncertainties are missing. Setting xmin=7.0 instead brings in a point with a valid σ, so it behaves as case 1.

@cadenmyers13

Copy link
Copy Markdown
Contributor Author

@sbillinge ready for review

@codecov

codecov Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.93496% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.33%. Comparing base (507aa4e) to head (38210a3).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
tests/test_profileparser.py 88.88% 4 Missing ⚠️
tests/test_contribution.py 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #242      +/-   ##
==========================================
- Coverage   84.95%   82.33%   -2.63%     
==========================================
  Files          44       47       +3     
  Lines        4886     5542     +656     
==========================================
+ Hits         4151     4563     +412     
- Misses        735      979     +244     
Files with missing lines Coverage Δ
tests/conftest.py 94.59% <100.00%> (+0.04%) ⬆️
tests/test_fitrecipe.py 100.00% <100.00%> (ø)
tests/test_fitresults.py 100.00% <100.00%> (ø)
tests/test_profile.py 100.00% <100.00%> (ø)
tests/test_contribution.py 99.57% <95.00%> (-0.43%) ⬇️
tests/test_profileparser.py 97.36% <88.88%> (-2.64%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cadenmyers13
cadenmyers13 marked this pull request as draft October 30, 2026 18:06
@cadenmyers13 cadenmyers13 changed the title fix: raise on uncertainties that are zero or NaN in FitContribution and warn in Profile fix: give points with invalid uncertainties no weight instead of dropping all uncertainties or making the residual infinite Oct 30, 2026
@cadenmyers13
cadenmyers13 marked this pull request as ready for review October 30, 2026 19:01
@cadenmyers13

Copy link
Copy Markdown
Contributor Author

okay this is ready for review again

@sbillinge

Copy link
Copy Markdown
Contributor

wow, I love this new way of doing it. Have Claude write all the test cases in more easily accessible and human-readable form. Let's make that a new group standard. That was so easy to follow. The only problem is that I didn't then check (a) whether the test was correctly implemented (I trusted Claude) nor (b) whether any other code was implemented that was not covered by that test. (trusted Claude on that too). But I can check both things in principle. It is a good approach to have Claude do this

@sbillinge
sbillinge merged commit 862979d into diffpy:main Oct 31, 2026
6 of 7 checks passed
@cadenmyers13
cadenmyers13 deleted the uncertainty-checks branch November 2, 2026 15:47
@cadenmyers13

Copy link
Copy Markdown
Contributor Author

@sbillinge Yeah I still do a pretty thorough review of the tests it writes to be safe and sure. I'd say the tests are good 97% of the time where the other 3% are unnecessary. I tried to get agents to do all that review for me but it burns through tokens lol...

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