speed up getMasses by looking up each distinct element once - #2265
Open
steps-re wants to merge 2 commits into
Open
speed up getMasses by looking up each distinct element once#2265steps-re wants to merge 2 commits into
steps-re wants to merge 2 commits into
Conversation
getMasses looped over every atom, calling str.capitalize twice and doing a dictionary lookup each time, although a structure contains only a handful of distinct element symbols. The lookup is now done once per distinct symbol and mapped back onto the atoms. getMasses runs on every PDB, mmCIF and MMTF structure load. Measured 6-13x faster with identical output: 3.59 -> 0.27 ms for a 12750 atom antibody, 291 -> 45 ms for a million atoms. Co-Authored-By: Claude <noreply@anthropic.com>
max(map(len, array), default=0) is equivalent and does the comparison in C. The default also covers the empty case, which the loop handled by leaving max_len at 0. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or 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
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.
disclosing up front, same as #2262: put together with help from claude, then reproduced, measured and verified by me before opening.
two performance fixes, both in code that runs on every structure load. no behaviour change in either.
1. getMasses looks up every atom individually
getMassesinprody/utilities/misctools.pyloops over every atom, callsstr.capitalize()twice per atom and does a dict lookup each time. but a structure only contains a handful of distinct element symbols, so nearly all of that work is repeated. switched it to resolve each distinct symbol once and map the result back onto the atoms withunique(..., return_inverse=True).this is on the path for every pdb, mmcif and mmtf load (
pdbfile.py,ciffile.py,mmtffile.pyall call it right after parsing elements), so it shows up on essentially any structure a user opens.measured on my machine, output identical in every case:
the unknown-symbol case still returns 0.0 rather than raising, the single-string case still returns a scalar, and case-insensitive matching is unchanged.
2. max_len loop in setData
prody/atomic/atomgroup.pyfinds the longest chain identifier with an explicit python loop.max(map(len, array), default=0)does the same in C, and thedefaultcovers the empty case the loop handled by leavingmax_lenat 0. small, but it runs on every chain assignment.verification
added
TestGetMassestoprody/tests/utilities/test_misctools.py: 7 tests covering known elements, case insensitivity, unknown symbols, repeated symbols (which is what would break if the unique/inverse mapping were wrong), empty input, single-string input and array input. i checked these have teeth by deliberately breaking the mapping and confirming they fail.ran
prody/tests/atomicandprody/tests/utilities: 618 passed.prody/tests/proteinshas 16 failures and 1 error on my machine, but they are identical on unpatched main, so they are pre-existing and unrelated to this change (mostlytest_instyand a network-dependent mmtf test).happy to split these into two prs if you'd rather take them separately.
mike