[5/5] Perf: index the fragment rows instead of the dense slots - #438
Open
GeorgWa wants to merge 1 commit into
Open
[5/5] Perf: index the fragment rows instead of the dense slots#438GeorgWa wants to merge 1 commit into
GeorgWa wants to merge 1 commit into
Conversation
`_parse_fragment` held the fragment number, the position and the precursor length of every dense slot as uint32, which cost 3 dense arrays of 4 bytes per slot. All three values are shared by every charged fragment type of a fragment row, so `_fill_in_indices` now writes one value per fragment row instead, and `_parse_fragment` no longer takes the tiled directions array. uint16 holds every value, because `max_frag_per_peptide` of `_fill_in_indices` bounds the row positions and the row counts at 300. `flatten_fragments` casts the kept `number` and `position` values back to uint32, so the flat columns keep their public dtype. `_calculate_fragment_numbers` takes a direction, a row position and a row count, and runs for the kept fragments only. The dense pass over every slot is gone, and so is the discarded output array it needed. Dropping its in-out `frag_number` parameter also gives direction 0 a defined result. `_fill_in_indices` writes the row count as a scalar instead of a product with an array of ones, which removes one allocation per precursor. `flatten_fragments` passes the intensities without a copy, because nothing writes to them any more. Zeroing the intensity of padding slots was a no-op, as padding is always excluded. Output stays bit-identical to main across 5400 generated configurations, with one exception: fragment rows that no precursor covers now report number 0 and position 0. main leaves those slots unwritten, because it allocates the arrays with `np.empty`, so their values were unspecified. At 48M dense slots, peak RSS drops from 1.78 GB to 1.32 GB, and from 3.47 GB on main. The call is ~30% faster than the previous step and ~45% faster than main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GeorgWa
changed the title
mschwoer
reviewed
Sep 1, 2026
Comment on lines
+957
to
+958
| row_positions = np.zeros(n_fragment_rows, dtype=np.uint16) | ||
| row_counts = np.zeros(n_fragment_rows, dtype=np.uint16) |
Contributor
There was a problem hiding this comment.
we intantiate here, then pass to _fill_in_indices .. why not instantiate there and return?
| ---------- | ||
| frag_direction : np.int8 | ||
| directions of fragments for each peptide | ||
| direction of the fragment type. 'abc' ions count from the first amino |
Contributor
There was a problem hiding this comment.
could there be string constants for 'abc' and 'xyz' ?
| return row_position + 1 | ||
| if frag_direction == -1: | ||
| return row_count - row_position | ||
| return 0 |
Contributor
There was a problem hiding this comment.
please add this behaviour to the docstring
| ) | ||
| needs_row = bool({"number", "position"}.intersection(custom_columns)) | ||
| kept_frag_types = kept_indices % n_fragment_types if needs_frag_type else None | ||
| kept_rows = kept_indices // n_fragment_types if needs_row else None |
Contributor
There was a problem hiding this comment.
please double check if a +1 is required due to flooring
| ) | ||
| if "position" in custom_columns: | ||
| columns["position"] = positions.reshape(-1)[kept_indices] | ||
| # the flat column keeps its uint32 dtype |
Contributor
There was a problem hiding this comment.
what extra information (compared to the next line) does this comment carry?
| # fills most dense slots. | ||
| mz = fragment_mz_df.values.reshape(-1) | ||
| use_intensity = len(fragment_intensity_df) > 0 | ||
| # `copy=False` keeps a dense copy out of memory whenever the dtype already |
Contributor
There was a problem hiding this comment.
"copy=False keeps a dense copy out of memory " .. wording is confusing (I guess aircraft operators were not trained in python, but maybe COBOL?)
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
2 participants
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.
Performance. One documented behaviour change, otherwise bit-identical to main.
parse_fragmentheld the number, the position and the precursor length of every dense slot.max_frag_per_peptidebounds.Behaviour change: fragment rows that no precursor covers now report number 0 and position 0.
mainleaves those slots unwritten (np.empty), so their values were unspecified.Peak RSS and runtime of the whole stack,
keep_top_k_fragments=12:Last of 5. Stacked on #437, and lands the same end state as #429.
🤖 Generated with Claude Code