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

Fix issue #330 #331

Closed
wants to merge 1 commit into from
Closed

Fix issue #330 #331

wants to merge 1 commit into from

Conversation

lohedges
Copy link
Contributor

This PR closes #330 by preserving SMILES based molecule properties when parameterising. Previously, BioSimSpace.Convert.smiles was only used to create an intermediate molecule during parameterisation, so any properties that were generated at this point were not part of the returned molecule. This PR fixes this by adding them to the molecule, hence preserving attributes that are required for stereochemistry.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@mb2055

@lohedges
Copy link
Contributor Author

Looks like this doesn't completely fix #330, rather the specific case of SMILES input. Note that the test that fails is passing locally. Will investigate further.

@lohedges
Copy link
Contributor Author

Okay, it works locally using the same environment as for the CI. The test involves generating a mapping with RDKit, so I had assumed that this had changed between versions. (It doesn't specify a conformer to use.) If it had failed on a single platform I would have put it down to bad luck, but here it's failed on all of them. (The test is skipped on Windows.)

@lohedges
Copy link
Contributor Author

Hmm, it seems to fail when all of the tests are run in one go, not individually. This would suggest some possible file caching issue, but the fixtures for this particular test are unique

@lohedges
Copy link
Contributor Author

It's definitely this PR that makes the test fail, but I can't see what is causing it. (The test passes in isolation, or even when run as part of the subset of the tests.) I feel like there is probably another bug that was masked somehow. Will keep digging.

@lohedges lohedges mentioned this pull request Oct 2, 2024
@lohedges lohedges closed this in #352 Oct 7, 2024
@lohedges lohedges deleted the fix_330 branch October 7, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] BSS.Convert.toRDKit() appears to lose stereochemical information
1 participant