Skip to content

Updated the CLI to use unbiased std for errors #2018

Merged
jthorton merged 1 commit into
mainfrom
cli_dof
Jun 17, 2026
Merged

Updated the CLI to use unbiased std for errors #2018
jthorton merged 1 commit into
mainfrom
cli_dof

Conversation

@jthorton

@jthorton jthorton commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Follow on from #2000 updates the CLI error functions to report the unbiased std as the error.

Questions

  • Do we want to update the gather CLIs to use the get_estimate and get_uncertainty protocol result functions to keep them in sync now or is this a 2.0 issue?
  • If not can we have a single util function which calculates the error and is used in the protocols and the CLI, isolating the behaviour to a single place?

Checklist

  • All new code is appropriately documented (user-facing code must have complete docstrings).
  • Added a news entry, or the changes are not user-facing.
  • Ran pre-commit: you can run pre-commit locally or comment on this PR with pre-commit.ci autofix.

Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).

Developers certificate of origin

@github-actions

Copy link
Copy Markdown

No API break detected ✅

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.60%. Comparing base (58d1e00) to head (f329068).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/openfecli/commands/gather_septop.py 0.00% 4 Missing ⚠️
src/openfecli/commands/gather_abfe.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2018      +/-   ##
==========================================
- Coverage   94.96%   90.60%   -4.36%     
==========================================
  Files         217      217              
  Lines       20725    20777      +52     
==========================================
- Hits        19681    18825     -856     
- Misses       1044     1952     +908     
Flag Coverage Δ
fast-tests 90.60% <37.50%> (?)
slow-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

@IAlibay IAlibay 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.

lgtm, thanks!

@IAlibay

IAlibay commented Jun 16, 2026

Copy link
Copy Markdown
Member

Do we want to update the gather CLIs to use the get_estimate and get_uncertainty protocol result functions to keep them in sync now or is this a 2.0 issue?

I think to do this you will need to load in the full Results object, my understanding is that we aren't doing this right now because there's some performance issues with it based on how it's currently stored in the quickrun outputs.

This is to say, I think it's a "new CLI issue", rather than something that can be dealt with today, but not necessary an 2.0 issue.

@IAlibay

IAlibay commented Jun 16, 2026

Copy link
Copy Markdown
Member

If not can we have a single util function which calculates the error and is used in the protocols and the CLI, isolating the behaviour to a single place?

This one I'm not sure I fully understand. Do you mean something like _get_stdev that does something like:

   std = np.std([v[0].m for v in r["overall"]], ddof=1)
    if np.isnan(std):
        std = 0.0
    return std

?

If so, I wouldn't be against having that as a small reusable method, as long as we're ok with it maybe being refactored in the near future (as we move towards squashing the protocols together).

@jthorton

jthorton commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

Do you mean something like _get_stdev that does something like:

Yesa single function which is called by the CLI and the protocols, I'll make an issue for that.

@atravitz

Copy link
Copy Markdown
Contributor

I think to do this you will need to load in the full Results object, my understanding is that we aren't doing this right now because there's some performance issues with it based on how it's currently stored in the quickrun outputs.

This is to say, I think it's a "new CLI issue", rather than something that can be dealt with today, but not necessary an 2.0 issue.

yes, this will require light breaking changes to quickrun - to be discussed in ResultsNetwork conversation tomorrow

@atravitz

Copy link
Copy Markdown
Contributor

Splitting off into a separate function is nice but not necessary - there's so much duplicated code I'm going to consolidate when merging everything into a single gather that this is the least of my worries.

@jthorton jthorton merged commit d967f7d into main Jun 17, 2026
9 of 10 checks passed
@jthorton jthorton deleted the cli_dof branch June 17, 2026 08:17
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.

3 participants