Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
mikemhenry
left a comment
mikemhenry
left a comment
There was a problem hiding this comment.
LGTM! Do we need to update our openfe docs to link to the renamed notebooks?
|
I think we need to bump the rdkit version? |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Overall lgtm, just the one thing about outputs.
Whilst is am saddened by the use of while of whilst, I agree that we decided on US over UK.
| "name": "stdout", | ||
| "output_type": "stream", | ||
| "text": [ | ||
| "Please cite the following:\n", |
There was a problem hiding this comment.
It's not a blocker, but as a user I generally prefer having this output there if possible. It helps me know that what is being written out is what should be expected.
There was a problem hiding this comment.
Sounds good - I generally agree but this was borderline too verbose. I think you're right in this instance because these warnings might confuse people otherwise.
There was a problem hiding this comment.
Oh I take it back - I intentionally commented out this block to avoid having a simulation run unless the user chooses to. This matches the behavior in the other notebooks.
yes! this PR: https://github.com/OpenFreeEnergy/openfe/compare/update_nb_links?expand=1 |
we already did this bump on conda forge, so I updated the env file here so the CI matches that. |
3 participants
No description provided.