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

Write multi-conformer PDBS with RDKit #579

Merged
merged 4 commits into from
Apr 16, 2020
Merged

Write multi-conformer PDBS with RDKit #579

merged 4 commits into from
Apr 16, 2020

Conversation

jthorton
Copy link
Collaborator

@jthorton jthorton commented Apr 14, 2020

This should fix RDKit and allow it to write multi-conformer PDB files, also added a test for this.

@jthorton
Copy link
Collaborator Author

This has made me realise I don't think we test all of the possible file output formats which I could add test for here if required?

@mattwthompson
Copy link
Member

Which formats are you referring to? We are in the process of adding some in #569

@jthorton
Copy link
Collaborator Author

Ahh yes I mean backend toolkit formats more like PDB and SDF I know SDF is more heavily tested now that the property PR is merged.

@j-wags
Copy link
Member

j-wags commented Apr 15, 2020

This has made me realise I don't think we test all of the possible file output formats which I could add test for here if required?

I'm starting to appreciate how "output formats" are complicated. That is, writing to a particular output format isn't just "making a valid file within that format's specification", but rather "deciding on exactly which characteristics of a molecule we guarantee to preserve in a round trip to that format".

So, this doesn't mean "don't add tests for new formats" -- after all, we do "support" them in the API, and testing is better than no testing. I mean something more like "don't be surprised if this task is a rabbit hole, and we end up with a big table of what we do and don't preserve through trips to different formats"

So, this PR shouldn't add tests for every output format, because it will take a long time.

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.

Thanks for updating the SDF tests to use StringIO -- That seems like a better practice, I was just super lazy when I first did it.

This PR looks good. Thanks for adding a test for it as well.

@j-wags
Copy link
Member

j-wags commented Apr 15, 2020

Oh, and please update the release notes before you merge!!

@jthorton
Copy link
Collaborator Author

Good point I had a feeling it may be a bigger job than I expected Ill get this finished up today.

@jthorton jthorton merged commit d02be7b into master Apr 16, 2020
@jthorton jthorton deleted the rdkit_pdbs branch April 16, 2020 10:19
@mattwthompson mattwthompson mentioned this pull request Apr 20, 2020
4 tasks
# 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.

RDKit cannot write multi-model PDB files
4 participants