Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

simple duration printout #294

Merged
merged 3 commits into from
Apr 10, 2023
Merged

simple duration printout #294

merged 3 commits into from
Apr 10, 2023

Conversation

RiesBen
Copy link
Contributor

@RiesBen RiesBen commented Apr 4, 2023

This pr brings simple duration timings to the cmd line tools:

  • plan_rbfe_network
  • plan_rhfe_network
  • quickrun

adds this line to the output: Duration: 0:00:30.134822
it is a super small code change, that adds a decorator to the cli tools.

Copy link
Contributor

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

so currently write will print to stdout, but this will make stdout noisy if you're also using that for the actual output of the command

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (a306ea5) 91.64% compared to head (86412c1) 91.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #294      +/-   ##
==========================================
+ Coverage   91.64%   91.66%   +0.02%     
==========================================
  Files          92       92              
  Lines        5662     5678      +16     
==========================================
+ Hits         5189     5205      +16     
  Misses        473      473              
Impacted Files Coverage Δ
openfecli/commands/plan_rbfe_network.py 100.00% <100.00%> (ø)
openfecli/commands/plan_rhfe_network.py 100.00% <100.00%> (ø)
openfecli/commands/quickrun.py 100.00% <100.00%> (ø)
openfecli/utils.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

return result

return wrapper

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need something as complicated as how openmmtools does it: https://github.com/choderalab/openmmtools/blob/main/openmmtools/utils/utils.py#L65-L183

But I much rather see the timing information written to a log level (maybe info?) so that a user can control where the output goes using either standard python logging tooling or posix things like 2&> /dev/null

Copy link
Contributor

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

I opened OpenFreeEnergy/gufe#168 which I think is a better system that @mikemhenry is alluding to. This is good enough and a nice improvement for 0.7 though

@richardjgowers richardjgowers enabled auto-merge (rebase) April 10, 2023 09:25
@richardjgowers richardjgowers merged commit 205a760 into main Apr 10, 2023
@richardjgowers richardjgowers deleted the easyTiming branch April 10, 2023 09:51
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.

4 participants