-
Notifications
You must be signed in to change notification settings - Fork 28
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
Coordgen is slower than RDKit's native 2d coordinate generation #39
Comments
@d-b-w, it might be a good idea to add some of the molecules (especially the slow ones) from these benchmarks as tests in this repository. |
Sorry for reviving this 2-year old ticket. I have just stumbled on the same problem on an internal dataset using the latest RDKit 2021.03.1 release. Native RDKit depiction of these 2000 molecules takes ~3 s:
At the moment, this means that |
ugh, we just accidentally blew up coordgen time by at least 10x, which should be addressed in - #90 Sorry about that. When #90 is merged, I'll immediately issue a patch release of coordgen and post a PR to RDKit. We're definitely hoping to do further work on this before the fall RDKit release. The bug in #90 actually provides some clues to next steps. |
Thank you for the super-fast reply, Dan! Looking forward to the PR. |
Thanks Dan! It looks much better now :-)
|
great! This issue is should remain open; I feel like the current rate is still too slow. But it's acceptable for many use cases. |
Coordgen is slower than RDKit's native 2d coordinate generation. Average speeds are about 100x slower, and in the worse cases, coordgen can take multiple seconds.
The two tools don't do the same things, and I think that coordgen results are much better, so the comparison is not totally fair. I do think that that coordgen should target being able to consistently produce coordinates in less than 0.1s, and have averages closer to 0.001s. This will allow us to discuss making coordgen the default in RDKit, which would be cool.
I'm going to link to the internal Schrödinger bug tracker, and our internal display for performance testing below, sorry...
At the time I post this, our automated performance testing says that:
The text was updated successfully, but these errors were encountered: