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

RDKitConverter improvements #4082

Merged
merged 9 commits into from
May 26, 2023

Conversation

cbouy
Copy link
Member

@cbouy cbouy commented Mar 19, 2023

Part of #3996

Changes made in this Pull Request:

  • moved some variables (MONATOMIC_CATION_CHARGES and STANDARDIZATION_REACTIONS) out of the related functions to allow users fine tuning them if necessary.
  • changed the sorting of heavy atoms when inferring bond orders and charges: previously only based on the number of unpaired electrons, now based on this and the number of heavy atom neighbors.
  • use RDKit's RunReactantInPlace for the standardization reactions, which should result in a significant speed improvement as we don't need to use bespoke code to transfer atomic properties from the non-standardized mol to the standardized one.
  • added more test cases for bond order and charges inferring for molecules that failed in the benchmark previously.

Ideally I need to run the benchmark again to make sure that their are no regression from these changes, and I'd also like to make a comparison between the develop branch and these changes as there should be a significant speed improvement from using RunReactantInPlace.

PR Checklist

  • Tests?
  • CHANGELOG updated?
  • Issue raised/referenced?

📚 Documentation preview 📚: https://readthedocs-preview--4082.org.readthedocs.build/en/4082/

@codecov
Copy link

codecov bot commented Mar 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.07 ⚠️

Comparison is base (e63c396) 93.65% compared to head (dc5f92a) 93.59%.

❗ Current head dc5f92a differs from pull request most recent head 65601de. Consider uploading reports for the commit 65601de to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4082      +/-   ##
===========================================
- Coverage    93.65%   93.59%   -0.07%     
===========================================
  Files           14      192     +178     
  Lines         1104    25109   +24005     
  Branches         0     4050    +4050     
===========================================
+ Hits          1034    23500   +22466     
- Misses          70     1092    +1022     
- Partials         0      517     +517     
Impacted Files Coverage Δ
package/MDAnalysis/converters/RDKit.py 98.97% <100.00%> (ø)

... and 178 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cbouy cbouy force-pushed the rdkitconverter-improvements branch from de4e537 to 8486bcd Compare March 19, 2023 22:02
@github-actions
Copy link

github-actions bot commented Mar 29, 2023

Linter Bot Results:

Hi @cbouy! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/5089795892/jobs/9147847656


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

@IAlibay IAlibay requested a review from richardjgowers March 29, 2023 22:52
Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

thanks @cbouy ! these changes look sane to me. I'd be curious how this approach compares to the new rdDetermineBond functions, but I think that's a long conversation to be had elsewhere

@richardjgowers richardjgowers merged commit eba3633 into MDAnalysis:develop May 26, 2023
@cbouy
Copy link
Member Author

cbouy commented May 26, 2023

Yes it would be nice to have it as an alternative bond-inferring solution as well, and try it on the benchmark that I had set up some time ago. Might give it a go someday

@cbouy cbouy deleted the rdkitconverter-improvements branch May 26, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants