-
Notifications
You must be signed in to change notification settings - Fork 35
Move rfe protocol #1769
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
Move rfe protocol #1769
Conversation
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
pre-commit.ci autofix |
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
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.
I might be being a bit premature with this one - I haven't done this move with the AFE protocols. I was partly being motivated by the fact that eventually we might have "full cycle" protocols that do the whole RHFE/RBFE, they will mostly inherit the same base class and have lightweight changes to their validation & construction methods. In that case it makes sense to have them live in a separate file somewhere so it's easier to see the subclasses.
hannahbaumann
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.
Thanks @IAlibay lgtm! Just two comments. Do we have a document somewhere where we're listing all the ideas for changes for the 2.0 or is that too premature?
| Acknowledgements | ||
| ---------------- | ||
| These ProtocolUnits are based on, and leverage components originating from |
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.
I wonder whether the file name could also be hybridtop_protocol_units.py to be more similar to the other files?
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.
That's a pretty good idea - I've added it to #1785. I might circle back and make the change once we're done with all the other PRs for the hybridtop protocol (changing this file name specifically will cause some headaches 😓 ).
Co-authored-by: Hannah Baumann <[email protected]>
|
No API break detected ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1769 +/- ##
==========================================
- Coverage 95.40% 93.21% -2.20%
==========================================
Files 190 193 +3
Lines 16818 16838 +20
==========================================
- Hits 16046 15695 -351
- Misses 772 1143 +371
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:
|
Blocked by #1740 and #1768
Towards protocol continuations - moves some files around to make things easier to work it.
IMPORTANT: This is not a finalized layout for all the files, so please don't lose time bikeshedding here - we can reformat everything later (especially as we move towards a 2.0).
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofixbefore requesting review.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