Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improved support for hierarchy schemes from OpenMM and PDBs #1404

Merged
merged 11 commits into from
Sep 22, 2022

Conversation

Yoshanuikabundi
Copy link
Collaborator

@Yoshanuikabundi Yoshanuikabundi commented Sep 21, 2022

This adds residue and chain hierarchy scheme support to Molecule.from_pdb_and_smiles and Topology.from_openmm.

This is based on #1400 to avoid conflicts, so that should be merged first.

Fixes #1386.

@Yoshanuikabundi Yoshanuikabundi marked this pull request as draft September 21, 2022 10:32
@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Merging #1404 (1f52f1a) into main (4a7a16c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@j-wags
Copy link
Member

j-wags commented Sep 21, 2022

Looking great - I think this is the right direction to go in. Adding hierarchy schemes in code paths where users are loading from PDB seems helpful at best and harmless at worst!

@Yoshanuikabundi Yoshanuikabundi force-pushed the pdb_and_smiles_residue_support branch from e53f2ad to 91d231a Compare September 22, 2022 05:39
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Yoshanuikabundi Yoshanuikabundi force-pushed the pdb_and_smiles_residue_support branch from 5d7df8e to f8d6b01 Compare September 22, 2022 08:56
@Yoshanuikabundi Yoshanuikabundi marked this pull request as ready for review September 22, 2022 09:05
@Yoshanuikabundi
Copy link
Collaborator Author

OK, I've added tests for the new behavior, updated the toolkit showcase to no longer use ensure_unique_atomnames=False, and fixed the broken JSON serialization of tuples in hierarchy schemes that was causing the examples to fail. I also noticed that the showcase still had widget states saved, even though the output had been cleared, so I cleaned that up too.

I think this is ready to go - tests are passing locally but I'll check later tonight that CI is happy! And I'm happy for this to be merged while I'm asleep, or I can merge it tomorrow.

Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, @Yoshanuikabundi. The tuple thing is a good improvement but I was a bit confused seeing it without context - In the future it may be good to keep urgent PRs minimal to reduce overhead on review :-)

Comment on lines +517 to +519
assert all(
all([molecule.residues, molecule.chains]) for molecule in topology.molecules
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not blocking) I don't understand the purpose of this - Is it checking that there are no Nones in any of the HierarchyElements? Or no (None, None, None, None) HierarchyElements? If the iterators weren't exposed at all this wouldn't need an assert since it would cause an AttributeError. This could use a comment explaining it.

@j-wags j-wags merged commit 1c571c3 into main Sep 22, 2022
@j-wags j-wags deleted the pdb_and_smiles_residue_support branch September 22, 2022 22:29
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hierarchy schemes are lost by Topology.from_openmm
2 participants