-
Notifications
You must be signed in to change notification settings - Fork 35
add support for one repeat per json #1076
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1076 +/- ##
==========================================
- Coverage 94.62% 92.91% -1.71%
==========================================
Files 136 137 +1
Lines 10087 10112 +25
==========================================
- Hits 9545 9396 -149
- Misses 542 716 +174
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@atravitz based on our conversation yesterday - I won't review this since it's in draft but do let me know if you want a review. |
78f07c8 to
b390c3a
Compare
IAlibay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to expand the help message for that new flag - so that it's clearer to someone why they might want to modify this value.
Associated with this, we probably would want a cookbook or a tutorial demonstrating this new approach? (separate issue though)
openfe/setup/alchemical_network_planner/relative_alchemical_network_planner.py
Show resolved
Hide resolved
IAlibay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm, but a couple things documentation wise.
@hannahbaumann @jthorton could you have a look here and make sure you're happy with the user facing docs?
openfe/setup/alchemical_network_planner/relative_alchemical_network_planner.py
Show resolved
Hide resolved
IAlibay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but just the one word change - going back a little bit to what @jthorton originally proposed wording wise.
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
6c0e5e0 to
848f350
Compare
|
No API break detected ✅ |
Resolves #1071
The intention is to change the default behavior to be
--n-protocol-repeats=1in v2.0, and for now this change allows for users to control this parameter via a CLI flag.Checklist
newsentryDevelopers certificate of origin