Update performance test and README to include more fair comparison with pubchempy #14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…slateable using both OPSIN and PubChem
…lightly softened language
JacksonBurns
requested changes
Jul 10, 2023
README.md
Outdated
| ## Speedup 50x from `pubchempy` | ||
| `py2opsin` runs locally and is smaller in scope in what it provides, which makes it __dramatically__ faster at resolving identifiers. In the code block below, the call to `py2opsin` will execute ~58x faster than an equivalent call to `puchempy`: | ||
| ## Massive speedup from `pubchempy` for batch translations | ||
| `py2opsin` runs locally and is smaller in scope in what it provides, which makes it __dramatically__ faster at resolving identifiers. In the code block below, the call to `py2opsin` will execute ~23 faster than an equivalent call to `pubchempy`: |
There was a problem hiding this comment.
Let's get rid of the numerical comparison here - going to vary based on chemicals, machines, internet speed, etc.
There was a problem hiding this comment.
OK, fixed that up in the latest commits
|
It looks like the performance tests have the possibility of failing if the PubChem server can't be reached. We may want to include error handling |
|
I think this might actually be a limitation of the GitHub runners - they are notoriously weird. On the second run, it passed. |
|
Ah, I spoke too soon - great! |
JacksonBurns
approved these changes
Jul 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes two small changes -
Updates the compound list in
test_performance.pyto include only 100 molecules that are known to be translateable using both OPSIN and PubChem. The previous list of compounds had many species that couldn't be resolved using PubChem, resulting in a potentially unfair comparison between the two tools. Also, the compound list was fairly long - ideally we want to keep the test list as short as possible so as to not spam the PubChem server.Updates the README to reflect this change, and modifies the language describing the comparison. In particular, the speedup will depend on the size of the input.
py2opsinhas a non-negligible startup time, and if you are only translating 1 species at a time,py2opsinperformance is actually comparable to querying PubChem. The real speedup comes from the batch translations, which is now indicated in the README.