Skip to content
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

Add interface to RDKit #736

Merged
merged 4 commits into from
Jan 23, 2025
Merged

Add interface to RDKit #736

merged 4 commits into from
Jan 23, 2025

Conversation

padix-key
Copy link
Member

@padix-key padix-key commented Jan 14, 2025

As described in #709, this PR adds an interface to RDKit and also sets the design of other future biotite.interface subpackages. Let me know what you think, especially the design on the higher level (e.g. the @requires_version() decorator, adding packaging as dependency, the nomenclature with from_* and to_*)

@t0mdavid-m @JHKru @MaxGreil

Copy link

codspeed-hq bot commented Jan 14, 2025

CodSpeed Performance Report

Merging #736 will not alter performance

Comparing padix-key:rdkit (c178d7c) with main (fab175e)

Summary

✅ 59 untouched benchmarks

@padix-key padix-key force-pushed the rdkit branch 4 times, most recently from 0ff9c1e to b84a7db Compare January 18, 2025 16:10
@padix-key padix-key changed the base branch from main to interfaces January 18, 2025 16:13
@padix-key padix-key force-pushed the rdkit branch 2 times, most recently from d7a3731 to abedbff Compare January 23, 2025 14:36
@padix-key padix-key merged commit a778010 into biotite-dev:interfaces Jan 23, 2025
28 checks passed
Comment on lines +120 to +121
for annot_name in include_annotations:
rdkit_atom.SetProp(annot_name, atoms.get_annotation(annot_name)[i].item())
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great @padix-key !

I did some digging and a small point here that might be convenient for the future:
RDKit has some reserved fields for residue and chain level information from PDB structures and we could set those standard field instead of arbitrary properties where those exist (e.g. res_name, res_id, chain_id):

        # use rdkit-native annotations for 'res_name', 'chain_id', 'res_id'
        res_info = Chem.AtomPDBResidueInfo()
        res_info.SetResidueName(atoms.res_name[i])
        res_info.SetResidueNumber(int(atoms.res_id[i]))
        res_info.SetChainId(atoms.chain_id[i])
        rdkit_atom.SetPDBResidueInfo(res_info)

The upside of this would be interoperability with tools like prolif that allow annotating interaction information

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok reading the documentation there's an even easier way to set it directly in one go, e.g.

Chem.AtomPDBResidueInfo(atomName="CA", residueName="ALA", chainId="A", residueNumber=1, occupancy=1.0, isHeteroAtom=False, insertionCode="")

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spotting, I missed there is a dedicated location for this. I agree we should use AtomPDBResidueInfo instead and only use include_annotations for optional catgegories were we would need to use SetProp.

Would like to create a follow-up PR for this? Note that the upstream branch needs to be interfaces, as the RDKit interface is not in main yet. The reason is that I aim to bundle the release of the interfaces subpackage together with the PyMOL and OpenMM interface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants