-
Notifications
You must be signed in to change notification settings - Fork 5
[WIP] clean up notebooks #200
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
base: main
Are you sure you want to change the base?
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
=======================================
Coverage 92.24% 92.24%
=======================================
Files 84 84
Lines 2618 2618
=======================================
Hits 2415 2415
Misses 203 203 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
docs/tutorials/building_networks.rst
Outdated
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.
this is now covered by updates made here: OpenFreeEnergy/ExampleNotebooks#274
This is all very openfe user-facing, so I think it should live in ExampleNotebooks, and then the more in-depth network_generation_algorithms can live in konnektor.
| @@ -0,0 +1,839 @@ | |||
| { | |||
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.
Line #6. Chem.Draw.MolsToGridImage([c.to_rdkit() for c in charge_components], molsPerRow=6)
It's hard to see the difference in the ligands and work out which are charged using this 3D view. Maybe try a 2D embedding with constant alignment and label the ligands with their formal charge?
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
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.
Line #6. concatenator = MstConcatenator(
Does this work it seems to have raised an error?
Reply via ReviewNB
| @@ -0,0 +1,839 @@ | |||
| { | |||
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.
As the redundant MST is not designed with cycles in mind, it might be worth pointing out specific cyclic graph generators, such as LOMAP, after discussing this aspect of the networks.
Reply via ReviewNB
PR Author Checklist
pre-commit.ci autofixto auto-format your PR).Tips
pre-commit.ci autofixto have pre-commit.ci auto-format your PR.Since this will create a commit, it is best to make this comment when you are finished with your work.
Developers certificate of origin
Reviewer Checklist