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

Validate compounds by looking for the "symptoms" of OpenBabel issues #104

Merged
merged 12 commits into from
May 9, 2024

Conversation

Aariq
Copy link
Collaborator

@Aariq Aariq commented Mar 18, 2024

Addresses #85 (and also closes #56) by checking the output of ChemmineR::propOB() for the symptoms of OpenBabel issues parsing SDFs. Why propOB()? Simply because that is the only ChemmineR function we have found that actually complains about problems in .mol files.

Currently this is tested with two cases:

  • A .mol file with a malformed header block
  • Posphotidylcholine, a molecule with an "R" group that is not recognized by OpenBabel.

I'm happy to have this PR merged as-is or add additional tests—I'm sure there are other ways that inputs (.mol files or SMILES) can be just broken enough to trigger OpenBabel errors, but not R errors, and return incorrect results.

Unfortunately, since InChI can't be generated with the windows version of ChemmineOB, this validation option can't be used on Windows. This is documented in the help file for get_fx_groups() and a warning is printed when validate = TRUE is used on Windows.

Still to do?:

  • Add something about validate to Getting Started vignette

@Aariq Aariq linked an issue Mar 18, 2024 that may be closed by this pull request
@Aariq Aariq requested a review from KristinaRiemer March 18, 2024 22:23
@Aariq
Copy link
Collaborator Author

Aariq commented Mar 18, 2024

It seems like the built-in version of OpenBabel included in the windows version of ChemmineOB might behave differently when it comes to creating the formula and InChI that we are using to diagnose parsing issues. Need to add some tests and investigate.

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 19, 2024

It looks like this strategy doesn't currently work on Windows because the windows version of ChemmineR::propOB() doesn't generate InChI. I've opened an issue: girke-lab/ChemmineOB#37.

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 20, 2024

ChemmineOB can't generate InChI on Windows girke-lab/ChemmineOB#37 (comment)

For now I may just only allow the validate argument to work on non-windows operating systems.

@Aariq Aariq marked this pull request as draft March 20, 2024 00:11
@Aariq Aariq marked this pull request as ready for review March 22, 2024 18:07
@Aariq
Copy link
Collaborator Author

Aariq commented Apr 26, 2024

ChemmineOB currently is encountering errors when being built from source on macOS: girke-lab/ChemmineOB#35.

Copy link
Collaborator

@KristinaRiemer KristinaRiemer left a comment

Choose a reason for hiding this comment

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

This looks good to me! I don't have any comments, thanks for doing all of this work @Aariq.

@Aariq
Copy link
Collaborator Author

Aariq commented May 9, 2024

Ok, going to go ahead and merge despite checks on macOS failing since I've confirmed checks work locally on macOS with binary version of ChemmineOB. Might do a separate PR to force GH actions to install ChemmineOB from binary rather than source if I can figure out how to do that easily.

@Aariq Aariq merged commit 3d315fb into master May 9, 2024
6 of 7 checks passed
@Aariq Aariq deleted the validate-ob branch May 9, 2024 20:12
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.

Check for invalid SDFs Capture OpenBabel errors and add to output of calc_vol()
2 participants