Skip to content

Pad zero uncertainties in the MLE solver#177

Open
jthorton wants to merge 7 commits intomainfrom
svd_padding
Open

Pad zero uncertainties in the MLE solver#177
jthorton wants to merge 7 commits intomainfrom
svd_padding

Conversation

@jthorton
Copy link
Contributor

@jthorton jthorton commented Jan 20, 2026

Description

Fixes #97 by padding calculated errors of exactally zero by some small amount.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • TODO 1

Questions

  • Question1

Checklist

  • Added a news entry for new features, bug fixes, or other user facing changes.

Status

  • Ready to go

Tips

  • Comment "pre-commit.ci autofix" to have pre-commit.ci atomically format your PR.
    Since this will create a commit, it is best to make this comment when you are finished with your work.

@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.67%. Comparing base (0899b64) to head (acb623b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
+ Coverage   95.64%   95.67%   +0.03%     
==========================================
  Files          18       18              
  Lines        1263     1273      +10     
==========================================
+ Hits         1208     1218      +10     
  Misses         55       55              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jthorton
Copy link
Contributor Author

pre-commit.ci autofix

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Not 100% sure about the impact, but adding this padding should be ok?

@jthorton jthorton requested a review from IAlibay January 27, 2026 12:00
# if the uncertainty is zero, the MLE solver will fail
# set all values to a small value if exactly zero
# see <https://github.com/OpenFreeEnergy/cinnabar/issues/97> for details
for i in range(N):
Copy link
Member

Choose a reason for hiding this comment

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

Should we be warning users that this is happening / we're fixing their data on the fly? In many ways the 1e-6 is likely going to come from cases where only a single repeat was used, so maybe we should warn folks that this isn't a great situation (given that MLE does use the errors).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could be warning but how many people listen to warnigs, maybe the alternative would be better, and we should raise an error if we detect this case, causing the user to handle this properly? They could of course just pad the values themselves in that case, but at least they know what is happening.

Copy link
Member

Choose a reason for hiding this comment

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

Not super sure on this one unfortunately. Would be worth a wider discussion?

cc @hannahbaumann what do you think?

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Sorry that was more of a lightweight request changes.

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.

Error of exactly zero will trigger SVD failure when generating absolute values

2 participants