-
Notifications
You must be signed in to change notification settings - Fork 51
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
Added function to get the number of stereoisomers #217
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
- Coverage 91.96% 91.93% -0.03%
==========================================
Files 46 46
Lines 3832 3843 +11
==========================================
+ Hits 3524 3533 +9
- Misses 308 310 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Lu.
It looks good to me after fixing the docstring.
Question: my understanding is that GetStereoisomerCount
will actually do that exact same as enumerate_stereoisomers
with n_variants=<MAX>
and simply call len()
on the output. Am I correct here? Maybe check what the rdkit code is doing under the hood. Not really a big deal for me here but I just wanted to flag it in case you think count()
should instead reuse enumerate()
.
rationalise: If we should try to build and rationalise the molecule to ensure it | ||
can exist. | ||
clean_it: A flag for assigning stereochemistry. If True, it will remove previous stereochemistry | ||
markings on the bonds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the CI is failing because of the malformed docstring. Locally, you can call mkdocs serve
to reproduce the error and it can help to fix the docstring.
Initially, I was using the output of I will also add an option to count the isomer using |
ok, so it seems like |
Changelogs
datamol.isomers._enumerate.count_stereoisomers
The step
Chem.FindPotentialStereoBonds(mol, cleanIt=clean_it)
, the information on bond is cleared ifcleanit=True
.Therefore,
cleanit
should be disabled when performing enumeration or counting only on undefined stereochemistry when the molecules have defined stereo information on bonds.See example below:
Reproduce the error