Implement the workflow pipeline #114
Conversation
Codecov Report
@@ Coverage Diff @@
## master #114 +/- ##
==========================================
+ Coverage 98.14% 98.16% +0.01%
==========================================
Files 24 25 +1
Lines 1351 1634 +283
Branches 295 348 +53
==========================================
+ Hits 1326 1604 +278
- Misses 5 7 +2
- Partials 20 23 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@orbeckst I think this PR is ready for review. |
xiki-tempula
changed the title
This reverts commit 90d033a.
There was a problem hiding this comment.
This is a lot of stuff and adds a whole new thing (workflows) to alchemlyb. It looks interesting but this is too much to review in one piece.
Could you please break up the PR into small, independent pieces, eg
- visualization
- separate_dhdl() functionality
- workflow
This will make reviewing easier.
Introducing workflows is its own discussion, which will best be done on a dedicated PR. Maybe @dotsdl already has an opinion on this, too?
docs/visualisation/alchemlyb.visualisation.plot_convergence.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I am ok with your approach to use AutoMBAR instead of MBAR, thanks to the documentation that you added and the use of methods. I had a bunch of doc changes that you can just accept. Please make sure that CHANGELOG records all behavior changes and additions. There were a few small things from my last review that weren't addressed so I pinged you on them again.
It's been a massive PR but it's really almost there. Sorry for my patchy availability to review.
src/alchemlyb/workflows/abfe.py
Outdated
| if self.ignore_warnings: | ||
| self.logger.warning(msg) | ||
| else: | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
To be honest, as soon as I see "pragma: no cover" I know that eventually I'll see this pop up as not working as thought... can you at least manually trigger it and confirm that it does want it should be doing?
And it is important to keep the original exception.
src/alchemlyb/workflows/abfe.py
Outdated
| Plot the free energy change as a function of time in both directions, | ||
| with the specified number of points in the time plot. The number of time | ||
| points (an integer) must be provided. Specify as ``None`` will not do | ||
| the convergence analysis. Default: 10. |
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
|
@orbeckst Thanks for the review. I have
|
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
|
Looking really good, the only thing I'd like to change is the ValueError hiding exceptions from the I/O. Otherwise you convinced me to go with your approach or made all the changes. |
There was a problem hiding this comment.
I have oscillated back and forth on the exception handling and came down on your suggested solution but using OSError. I will make these changes and see if it works. I am not sure if you're actually testing the exception handling anywhere. (If yes, I'll see it soon... if no, could we add a test?)
|
@xiki-tempula congratulations — after 1.5 years and almost 300 comments, your ABFE is finally in!!!! 🚀 |
I will try to Implement the workflow pipeline as is discussed in #111 .
I noticed that the subsampling part will be refactored with #98. Given I cannot leave this part blank, I have made a minimal implementation of the corresponding option and I'm happy to change it when #98 is merged.
All the information is logged using logging #34.