Feature/fast clustering - #31
Open
Janne98 wants to merge 72 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors clustering to bucketed Lance datasets, computes memmapped condensed distance matrices in parallel, clusters per m/z interval with cost-based chunking and NaN-safe m/z/RT postprocessing, produces medoid/average consensus spectra (Numba-accelerated), replaces assignment similarity with greedy matching, updates I/O identifiers/RT handling, adds utils, and adds extensive tests. ChangesFalcon v2 Clustering and Consensus Spectrum Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR modernizes Falcon’s clustering pipeline by switching to hierarchical clustering over a full pairwise cosine distance matrix (Numba-parallel), adds consensus spectrum support (medoid/average with outlier rejection), and improves I/O + configuration around charge bucketing and representative export.
Changes:
- Replaced sparse NN/DBSCAN pipeline with hierarchical clustering over a condensed cosine distance matrix and a faster greedy peak matching similarity.
- Added consensus spectrum generation (
medoidandaverage+ sigma-clipping outlier rejection) and corresponding CLI/config options. - Reworked spectrum I/O (mzML/mzXML/MGF identifiers, NaN RT handling) and added extensive tests + updated README.
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
falcon/falcon.py |
Main orchestration updated for charge buckets, consensus export, and Lance dataset writing. |
falcon/config.py |
Adds CLI/config parsing for consensus options and precursor charge buckets. |
falcon/utils.py |
Centralizes seed + logger configuration (replaces seed.py usage). |
falcon/seed.py |
Removes legacy seed helper (migrated to utils.py). |
falcon/ms_io/ms_io.py |
Adjusts I/O dispatch behavior (removes legacy is_processed flag). |
falcon/ms_io/mgf_io.py |
Adds identifier normalization, representative writing, and intensity scaling for MGF export. |
falcon/ms_io/mzml_io.py |
Builds stable identifiers from mzML scan IDs; uses NaN RT default. |
falcon/ms_io/mzxml_io.py |
Builds stable identifiers; uses NaN RT default; normalizes charge type. |
falcon/cluster/spectrum.py |
Removes legacy vectorization code and unused fields (e.g., filename). |
falcon/cluster/similarity.py |
Replaces Hungarian assignment with greedy peak matching for speed. |
falcon/cluster/distance_matrix.py |
New parallel condensed distance matrix computation (Numba). |
falcon/cluster/consensus.py |
New consensus spectrum algorithms (medoid + averaging with outlier rejection). |
falcon/cluster/cluster.py |
Reworks clustering to use the new distance matrix + consensus selection. |
README.md |
Updates docs to match the new pipeline, parameters, and defaults. |
setup.cfg |
Bumps joblib minimum version. |
.gitignore |
Adds Python and local dev ignores. |
tests/conftest.py |
Adds shared fixtures for new clustering/consensus and Lance dataset tests. |
tests/test_config.py |
Adds config parsing/validation tests for new CLI options. |
tests/test_falcon.py |
Adds tests for charge bucketing and Lance writing/routing behavior. |
tests/test_ms_io.py |
Adds tests for dispatch + MGF read/write roundtrip and identifier behavior. |
tests/test_mzml_mzxml_io.py |
Adds tests for mzML/mzXML parsing and identifiers. |
tests/cluster/test_cluster.py |
Adds extensive clustering behavior tests (splits, RT handling, relabeling, end-to-end). |
tests/cluster/test_consensus.py |
Adds tests for consensus methods and outlier rejection utilities. |
tests/cluster/test_distance_matrix.py |
Adds tests for condensed indexing and distance computation. |
tests/cluster/test_similarity.py |
Adds tests for greedy cosine similarity behavior. |
tests/cluster/test_spectrum.py |
Adds tests for spectrum preprocessing pipeline. |
tests/data/sample.mzml |
Adds minimal mzML fixture for I/O tests. |
tests/data/sample.mzxml |
Adds minimal mzXML fixture for I/O tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
falcon/falcon.py (1)
486-496:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRace condition: lock is keyed by
chargebut dataset is keyed bybucket_key.Multiple charges can map to the same
bucket_key(e.g., charges 1 and 2 both map to bucket(1, 2)). The code acquires locks usinglance_locks.get(charge), but the dataset path is derived frombucket_key. This means:
- Thread handling charge=1 acquires lock for key
1- Thread handling charge=2 acquires lock for key
2- Both threads write to the same
spectra_charge_1_2.lancedataset concurrentlyThis can cause data corruption in the Lance dataset.
Proposed fix - use bucket_key for locking
for charge in spec_to_write.keys(): if len(spec_to_write[charge]) == 0: continue _write_to_dataset( spec_to_write[charge], charge, - lance_locks.get(charge), + lance_locks.get(charge), # Note: charge here IS the bucket_key schema, config.work_dir, )Wait - looking more carefully,
spec_to_writeis keyed bybucket_key, notcharge. So the iteration variablechargein line 486 is actually a bucket_key. But then the flush path at line 512 uses the originalchargevariable (the spectrum's charge), notbucket_key.The fix should be:
- _write_to_dataset( - spec_to_write[bucket_key], - bucket_key, - lance_locks.get(charge), - schema, - config.work_dir, - ) + _write_to_dataset( + spec_to_write[bucket_key], + bucket_key, + lance_locks.get(bucket_key), + schema, + config.work_dir, + )Also applies to: 508-516
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/falcon.py` around lines 486 - 496, The loop uses the wrong lock key: spec_to_write is keyed by bucket_key but the code calls lance_locks.get(charge) and later uses a different `charge` when building the dataset path/flush, causing concurrent writes to the same dataset; fix by treating the loop variable as bucket_key (rename if helpful), acquire lance_locks.get(bucket_key) and pass that lock to _write_to_dataset, and ensure the flush/filename logic and spec_to_write clearing all use the same bucket_key (not the original spectrum `charge`), applying the same change in both the earlier loop and the later flush block (places around _write_to_dataset, spec_to_write.clear(), and any code that constructs spectra_charge_* .lance).
🧹 Nitpick comments (16)
falcon/cluster/cluster.py (4)
815-815: 💤 Low valueAmbiguous Unicode character in comment.
The comment contains
×(MULTIPLICATION SIGN) which may display differently across editors. Consider usingx(lowercase letter) instead.- # Group reps by mz_split in one pass — O(R) instead of O(S × R). + # Group reps by mz_split in one pass -- O(R) instead of O(S x R).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/cluster.py` at line 815, Replace the ambiguous Unicode multiplication sign in the comment "Group reps by mz_split in one pass — O(R) instead of O(S × R)." with a plain ASCII 'x' so it reads "O(S x R)"; update the comment near the grouping logic that references mz_split and reps (the same line containing "Group reps by mz_split in one pass") to ensure consistent display across editors.
683-684: 💤 Low valueUnused loop variable.
The loop variable
iis not used within the loop body.- for i, label in enumerate(cluster_assignments): + for _, label in enumerate(cluster_assignments): labels[label] = labels.get(label, 0) + 1Or simply:
- for i, label in enumerate(cluster_assignments): + for label in cluster_assignments: labels[label] = labels.get(label, 0) + 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/cluster.py` around lines 683 - 684, The for-loop over cluster_assignments uses an unused loop variable i; update the loop in cluster.py so it doesn’t enumerate when not needed (replace "for i, label in enumerate(cluster_assignments)" with a simple "for label in cluster_assignments") and keep the body that updates labels (labels[label] = labels.get(label, 0) + 1) unchanged to avoid the unused variable warning.
264-272: ⚡ Quick winRemove or resolve commented-out code.
This commented block with
# TODO: ask wout about thisshould either be removed or the decision resolved before merge to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/cluster.py` around lines 264 - 272, Remove the commented-out block containing the "TODO: ask wout about this" logic around block_size, batch_size, splits, n_chunks and chunk_size: either implement the intended splitting behavior (use the shown algorithm to compute n_chunks, chunk_size and populate splits) or delete the entire commented section and the TODO so it doesn't remain in the code; if you choose to keep a short note, replace the TODO with a concise explanatory comment describing why the code was omitted and link to an issue/PR that will implement it.
145-161: 💤 Low valueConsider consolidating TODOs and cleaning up redundant data preparation.
The
row_idsandidx_intervalat lines 150-151 are computed from the same slice. If they serve different purposes (Lance row IDs vs. positional indices), consider adding a clarifying comment. Also, the TODOs at lines 127 and 145 indicate technical debt that could be addressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/cluster.py` around lines 145 - 161, The code in cluster.py is computing the same slice twice (row_ids = idx[interval_start:interval_stop] and idx_interval = idx[interval_start:interval_stop]) and has TODOs about moving this into a chunking method; fix by consolidating the duplicate slice into a single variable (e.g., interval_slice = idx[interval_start:interval_stop]) and then derive row_ids = interval_slice.tolist() and idx_interval = interval_slice (or a renamed variable) so intent is explicit, add a short clarifying comment explaining the difference between Lance row IDs vs positional indices, and move this data-preparation loop into a dedicated helper (e.g., a new chunking method used by data_chunks creation) to address the TODOs; update references to data_chunks, chunks, idx, mzs accordingly.falcon/cluster/consensus.py (5)
566-575: 💤 Low valueParameter type annotations are inconsistent with actual usage.
The
bins_peaksandbins_mzparameters are declared asnb.typed.Listbut are actuallynp.ndarrayafter processing through_outlier_rejection.def _construct_average_spectrum( bins_indices: List[int], - bins_peaks: nb.typed.List, - bins_mz: nb.typed.List, + bins_peaks: np.ndarray, + bins_mz: np.ndarray, avg_precursor_mz: float,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/consensus.py` around lines 566 - 575, The signature of _construct_average_spectrum incorrectly types bins_peaks and bins_mz as nb.typed.List while they are np.ndarray after _outlier_rejection; update the type annotations in the _construct_average_spectrum definition to use appropriate numpy array types (e.g., np.ndarray with element dtype) and adjust any internal assumptions accordingly, and ensure any callers (including _outlier_rejection) and numba usage remain compatible with the new np.ndarray annotations so the function and `@nb.njit` compile and operate with the actual array inputs.
74-76: 💤 Low valueDocstring formatting issue: misaligned parameter.
The
order_mapparameter description is incorrectly indented, appearing to be part of thertsparameter description rather than a separate entry.rts : np.ndarray The retention times corresponding to the current interval indexes. - order_map : np.ndarray + order_map : np.ndarray Map to convert label indexes to pairwise distance matrix indexes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/consensus.py` around lines 74 - 76, The docstring currently nests the `order_map` description under the `rts` parameter; update the docstring so `order_map : np.ndarray` is a separate parameter entry aligned with `rts` (same indentation/format as other params) and move its description to a new line directly under `order_map` so it’s not part of the `rts` paragraph; ensure parameter names `rts` and `order_map` are clearly separated and follow the same docstring style used elsewhere in this module.
487-493: ⚡ Quick winReturn type annotation is incorrect.
The function returns a tuple of two arrays (
intensities,mzs) but the type hint declares a singlenp.ndarray.-) -> np.ndarray: +) -> Tuple[np.ndarray, np.ndarray]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/consensus.py` around lines 487 - 493, The _sigma_clipping function is annotated to return a single np.ndarray but actually returns two arrays (intensities, mzs); update its return type to a tuple of two arrays (e.g., Tuple[np.ndarray, np.ndarray] or (np.ndarray, np.ndarray)) and add any required import (typing.Tuple) so the signature correctly reflects the returned (intensities, mzs) pair used by callers.
425-432: ⚡ Quick winReturn type annotation is incorrect.
The function returns a tuple of three numpy arrays, but the type hint incorrectly declares
Tuple[nb.typed.List].-) -> Tuple[nb.typed.List]: +) -> Tuple[np.ndarray, np.ndarray, np.ndarray]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/consensus.py` around lines 425 - 432, The annotated return type for _outlier_rejection is wrong: it declares Tuple[nb.typed.List] but the function actually returns three arrays. Update the signature to return a 3-tuple of typed lists, e.g. change the annotation to Tuple[nb.typed.List, nb.typed.List, nb.typed.List] (and keep the `@nb.njit`(cache=True) decorator); also verify the function body of _outlier_rejection still returns exactly three nb.typed.List objects to match the new annotation.
353-359: ⚡ Quick winReturn type annotation is incomplete.
The function returns three values (
bins_indices,bins_peaks_nb,bins_mz_nb) but the type hint only declares two.) -> Tuple[List[int], nb.typed.List]: +) -> Tuple[np.ndarray, nb.typed.List, nb.typed.List]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/consensus.py` around lines 353 - 359, The return type annotation for _spectrum_binning is wrong: the function returns three values (bins_indices, bins_peaks_nb, bins_mz_nb) but the signature only declares two. Update the def _spectrum_binning(...) -> Tuple[...] annotation to list all three return element types (e.g., Tuple[List[int], nb.typed.List, nb.typed.List] or the concrete typed.List element types you use) so the signature matches the actual returns; keep the three return variable names (bins_indices, bins_peaks_nb, bins_mz_nb) in mind when choosing the types.falcon/falcon.py (2)
374-404: 💤 Low valueDocstring parameter name mismatch.
The docstring says
charge : intbut the parameter is namedcharge_bucket.Proposed fix
def _create_lance_dataset( charge_bucket: int, schema: pa.Schema ) -> lance.LanceDataset: """ Create a lance dataset. Parameters ---------- - charge : int - The precursor charge of the spectra. + charge_bucket : Union[Tuple[int, ...], str] + The bucket key for the spectra. schema : pa.Schema The schema of the dataset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/falcon.py` around lines 374 - 404, The docstring for _create_lance_dataset has a parameter name mismatch: it documents "charge" but the function parameter is "charge_bucket"; update the docstring to use the correct parameter name "charge_bucket" and adjust the description accordingly so the Parameters section matches the function signature (refer to _create_lance_dataset and the parameter charge_bucket and schema).
31-33: 💤 Low valueConsider explicit type annotation for optional parameter.
The static analysis hint indicates PEP 484 prohibits implicit
Optional. While this is a minor style issue, it improves clarity.Proposed fix
-def main(args: Union[str, List[str]] = None) -> int: +def main(args: Union[str, List[str], None] = None) -> int:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/falcon.py` around lines 31 - 33, The function signature for main uses an implicitly optional parameter; update it to use an explicit Optional type annotation (e.g., change the args annotation to Optional[Union[str, List[str]]]) and add the corresponding import (Optional) from typing to the module so static type checkers accept it; locate the main function and the module-level typing imports to make these changes.Source: Linters/SAST tools
falcon/utils.py (1)
8-10: Consider replacing legacynp.random.seed()withnp.random.default_rng()for explicit RNG state. Infalcon/utils.py(lines 8-10),set_seeds()usesnp.random.seed(my_seed); a search fornp.random.*underfalcon/only found this call, but switching toGeneratorwould likely require propagating anrngobject if other code relies on NumPy’s global RNG.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/utils.py` around lines 8 - 10, The current set_seeds(my_seed=42) sets NumPy's global RNG via np.random.seed, which is legacy; change set_seeds to create and return an explicit Generator using rng = np.random.default_rng(my_seed) (keep random.seed(my_seed) for Python's random) and update call sites to accept/propagate that rng instead of relying on NumPy's global state (alternatively expose a module-level NUMPY_RNG = np.random.default_rng(my_seed) from falcon.utils and update callers to use that RNG). Ensure the function signature (set_seeds) and callers (wherever it is invoked) are updated to use the returned/exported rng object so code uses Generator methods instead of np.random.* globals.tests/cluster/test_cluster.py (3)
271-271: 💤 Low valueUse ASCII 'x' instead of Unicode multiplication sign in docstring.
The docstring contains "×" (MULTIPLICATION SIGN, U+00D7). Use ASCII "x" for better compatibility and to satisfy linter rules.
📝 Proposed fix
- Expected result: 4 clusters (2 m/z groups × 2 RT sub-groups each: + Expected result: 4 clusters (2 m/z groups x 2 RT sub-groups each:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cluster/test_cluster.py` at line 271, Replace the Unicode multiplication sign "×" in the docstring with the ASCII letter "x" to satisfy the linter and improve compatibility; locate the docstring in tests/cluster/test_cluster.py (the comment describing "Expected result: 4 clusters (2 × 2 ...") and change "2 × 2" to "2 x 2" (or any similar occurrence of "×") so the test docstring uses only ASCII characters.
79-79: 💤 Low valueRemove leftover debug print statement.
This print statement appears to be a debugging artifact and should be removed.
🧹 Proposed fix
linkage_matrix = cluster._linkage(values, "Da") assert linkage_matrix.shape == (4, 4) - print(linkage_matrix) assert (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cluster/test_cluster.py` at line 79, Remove the leftover debug print statement by deleting the call print(linkage_matrix) in the test (replace with nothing or, if needed for debugging, use logging at debug level or an assertion); locate the print(linkage_matrix) occurrence in the test_cluster test and remove that line so the test output is clean.
463-463: 💤 Low valuePrefix unused
repsvariable with underscore.The
repsreturn value is unpacked but never used in these tests. Prefix with_to indicate this is intentional and suppress linter warnings.🔧 Proposed fix
- labels, reps = cluster.generate_clusters( + labels, _reps = cluster.generate_clusters(Apply the same change to lines 510 and 540.
Also applies to: 510-510, 540-540
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cluster/test_cluster.py` at line 463, The test unpacks cluster.generate_clusters into labels, reps but never uses reps; rename the unused variable to _reps to signal intentional unused and silence linters. Update the three call sites where generate_clusters is unpacked (the occurrences using variables named reps at lines around the calls to cluster.generate_clusters) to use _reps instead of reps so tests remain functionally identical while satisfying the linter.tests/cluster/test_consensus.py (1)
166-179: 💤 Low valueConsider verifying the retention time as well.
The test asserts that the precursor m/z matches the first spectrum, but could also assert
retention_times[0] == 10.0to fully confirm the medoid selection logic for small clusters.✨ Suggested enhancement
assert len(precursor_mzs) == 1 assert precursor_mzs[0] == spectra[0].precursor_mz + assert retention_times[0] == 10.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cluster/test_consensus.py` around lines 166 - 179, Update the test_small_cluster unit test to also assert the medoid retention time: after calling consensus._get_cluster_medoids (in test_small_cluster) add an assertion that retention_times[0] == 10.0 (or use a small tolerance if floating-point precision is a concern) to confirm the chosen medoid’s retention time matches the first spectrum’s rts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@falcon/cluster/cluster.py`:
- Around line 540-544: The precursor_charge field is being cast to np.float64 in
the ConsensusTuple construction, which conflicts with ConsensusTuple's expected
np.int32 or np.nan and with the _get_representative_spectra behaviour; change
the cast where precursor_charge is set (the precursor_charge=(...) expression)
to produce either an np.int32 value or np.nan (mirror the logic used in
_get_representative_spectra) so the field consistently uses np.int32 for valid
integer charges and numpy.nan otherwise.
In `@falcon/cluster/distance_matrix.py`:
- Around line 33-49: compute_condensed_distance_matrix currently creates the
memmap inside a with NamedTemporaryFile which is closed/deleted on return,
invalidating condensed_dist_matrix; change to create the temp file with
NamedTemporaryFile(delete=False) (or use tempfile.mkstemp) and assign its path
to pdist_filename, construct the memmap (condensed_dist_matrix) from that path,
return the memmap, and add explicit cleanup logic so callers (or a
finally/cleanup function in this module) unlink the temp file path
(pdist_filename) after the memmap is closed; update references to
pdist_file/pdist_filename and ensure callers that delete pdist now unlink the
underlying temp file path.
In `@falcon/config.py`:
- Around line 241-284: In parse_and_validate_charge_buckets, the error message
builds f"Charge value(s) {sorted(overlap)}..." which can TypeError when overlap
mixes ints and "unknown"; change the construction to produce a stable,
comparable list by mapping values to strings before sorting (e.g.,
sorted(map(str, overlap)) or using a key=str) and include that joined/printed
representation in the argparse.ArgumentTypeError so the error shows the
offending values without raising a new exception.
In `@falcon/falcon.py`:
- Around line 502-516: The code currently uses bucket_key (derived from
charge_to_bucket and catch_other_charges) but then unconditionally accesses
spec_to_write[bucket_key], which can create a None key or cause downstream
errors; move the length check and subsequent _write_to_dataset call inside the
existing if bucket_key is not None: block so all uses of
spec_to_write[bucket_key] only happen when bucket_key is valid, and ensure you
pass lance_locks.get(charge), schema, and config.work_dir to _write_to_dataset
only from within that block to avoid handling None keys.
In `@falcon/ms_io/mzml_io.py`:
- Line 31: In get_spectra, avoid directly accessing f_in.name (which can raise
AttributeError for in-memory streams); instead detect whether f_in is a str path
or a file-like object, use os.path.splitext(os.path.basename(...)) when f_in is
a str, otherwise use getattr(f_in, "name", None) and fall back to a safe default
(e.g., "unknown" or None) when name is missing before assigning filename; update
the code around the filename assignment that currently reads filename =
os.path.splitext(os.path.basename(f_in.name))[0] to use this guarded approach.
In `@README.md`:
- Around line 75-76: Update the README entry for the `linkage` option so the
fragment becomes a complete sentence: replace "Should be one of `single`,
`complete`, or `average`." with a full sentence such as "It must be one of
`single`, `complete`, or `average`." Ensure the `linkage` description now reads
as a complete sentence including the default (`complete`) and the allowed
values.
In `@tests/cluster/test_spectrum.py`:
- Around line 130-145: The assertion in test_precursor_removal is allowing a
peak exactly at the precursor m/z due to the unnecessary "or abs(mz_val - 500.0)
== 0" clause; update the assertion in the test_precursor_removal function (which
calls spectrum.process_spectrum with remove_precursor_tolerance=1.5 and
simple_spectrum) to assert that every mz in result["mz"] is farther than the
tolerance from 500.0 by replacing the current condition with a single check that
abs(mz_val - 500.0) > 1.5 so peaks within tolerance (including exactly 500.0)
are considered removed.
---
Outside diff comments:
In `@falcon/falcon.py`:
- Around line 486-496: The loop uses the wrong lock key: spec_to_write is keyed
by bucket_key but the code calls lance_locks.get(charge) and later uses a
different `charge` when building the dataset path/flush, causing concurrent
writes to the same dataset; fix by treating the loop variable as bucket_key
(rename if helpful), acquire lance_locks.get(bucket_key) and pass that lock to
_write_to_dataset, and ensure the flush/filename logic and spec_to_write
clearing all use the same bucket_key (not the original spectrum `charge`),
applying the same change in both the earlier loop and the later flush block
(places around _write_to_dataset, spec_to_write.clear(), and any code that
constructs spectra_charge_* .lance).
---
Nitpick comments:
In `@falcon/cluster/cluster.py`:
- Line 815: Replace the ambiguous Unicode multiplication sign in the comment
"Group reps by mz_split in one pass — O(R) instead of O(S × R)." with a plain
ASCII 'x' so it reads "O(S x R)"; update the comment near the grouping logic
that references mz_split and reps (the same line containing "Group reps by
mz_split in one pass") to ensure consistent display across editors.
- Around line 683-684: The for-loop over cluster_assignments uses an unused loop
variable i; update the loop in cluster.py so it doesn’t enumerate when not
needed (replace "for i, label in enumerate(cluster_assignments)" with a simple
"for label in cluster_assignments") and keep the body that updates labels
(labels[label] = labels.get(label, 0) + 1) unchanged to avoid the unused
variable warning.
- Around line 264-272: Remove the commented-out block containing the "TODO: ask
wout about this" logic around block_size, batch_size, splits, n_chunks and
chunk_size: either implement the intended splitting behavior (use the shown
algorithm to compute n_chunks, chunk_size and populate splits) or delete the
entire commented section and the TODO so it doesn't remain in the code; if you
choose to keep a short note, replace the TODO with a concise explanatory comment
describing why the code was omitted and link to an issue/PR that will implement
it.
- Around line 145-161: The code in cluster.py is computing the same slice twice
(row_ids = idx[interval_start:interval_stop] and idx_interval =
idx[interval_start:interval_stop]) and has TODOs about moving this into a
chunking method; fix by consolidating the duplicate slice into a single variable
(e.g., interval_slice = idx[interval_start:interval_stop]) and then derive
row_ids = interval_slice.tolist() and idx_interval = interval_slice (or a
renamed variable) so intent is explicit, add a short clarifying comment
explaining the difference between Lance row IDs vs positional indices, and move
this data-preparation loop into a dedicated helper (e.g., a new chunking method
used by data_chunks creation) to address the TODOs; update references to
data_chunks, chunks, idx, mzs accordingly.
In `@falcon/cluster/consensus.py`:
- Around line 566-575: The signature of _construct_average_spectrum incorrectly
types bins_peaks and bins_mz as nb.typed.List while they are np.ndarray after
_outlier_rejection; update the type annotations in the
_construct_average_spectrum definition to use appropriate numpy array types
(e.g., np.ndarray with element dtype) and adjust any internal assumptions
accordingly, and ensure any callers (including _outlier_rejection) and numba
usage remain compatible with the new np.ndarray annotations so the function and
`@nb.njit` compile and operate with the actual array inputs.
- Around line 74-76: The docstring currently nests the `order_map` description
under the `rts` parameter; update the docstring so `order_map : np.ndarray` is a
separate parameter entry aligned with `rts` (same indentation/format as other
params) and move its description to a new line directly under `order_map` so
it’s not part of the `rts` paragraph; ensure parameter names `rts` and
`order_map` are clearly separated and follow the same docstring style used
elsewhere in this module.
- Around line 487-493: The _sigma_clipping function is annotated to return a
single np.ndarray but actually returns two arrays (intensities, mzs); update its
return type to a tuple of two arrays (e.g., Tuple[np.ndarray, np.ndarray] or
(np.ndarray, np.ndarray)) and add any required import (typing.Tuple) so the
signature correctly reflects the returned (intensities, mzs) pair used by
callers.
- Around line 425-432: The annotated return type for _outlier_rejection is
wrong: it declares Tuple[nb.typed.List] but the function actually returns three
arrays. Update the signature to return a 3-tuple of typed lists, e.g. change the
annotation to Tuple[nb.typed.List, nb.typed.List, nb.typed.List] (and keep the
`@nb.njit`(cache=True) decorator); also verify the function body of
_outlier_rejection still returns exactly three nb.typed.List objects to match
the new annotation.
- Around line 353-359: The return type annotation for _spectrum_binning is
wrong: the function returns three values (bins_indices, bins_peaks_nb,
bins_mz_nb) but the signature only declares two. Update the def
_spectrum_binning(...) -> Tuple[...] annotation to list all three return element
types (e.g., Tuple[List[int], nb.typed.List, nb.typed.List] or the concrete
typed.List element types you use) so the signature matches the actual returns;
keep the three return variable names (bins_indices, bins_peaks_nb, bins_mz_nb)
in mind when choosing the types.
In `@falcon/falcon.py`:
- Around line 374-404: The docstring for _create_lance_dataset has a parameter
name mismatch: it documents "charge" but the function parameter is
"charge_bucket"; update the docstring to use the correct parameter name
"charge_bucket" and adjust the description accordingly so the Parameters section
matches the function signature (refer to _create_lance_dataset and the parameter
charge_bucket and schema).
- Around line 31-33: The function signature for main uses an implicitly optional
parameter; update it to use an explicit Optional type annotation (e.g., change
the args annotation to Optional[Union[str, List[str]]]) and add the
corresponding import (Optional) from typing to the module so static type
checkers accept it; locate the main function and the module-level typing imports
to make these changes.
In `@falcon/utils.py`:
- Around line 8-10: The current set_seeds(my_seed=42) sets NumPy's global RNG
via np.random.seed, which is legacy; change set_seeds to create and return an
explicit Generator using rng = np.random.default_rng(my_seed) (keep
random.seed(my_seed) for Python's random) and update call sites to
accept/propagate that rng instead of relying on NumPy's global state
(alternatively expose a module-level NUMPY_RNG = np.random.default_rng(my_seed)
from falcon.utils and update callers to use that RNG). Ensure the function
signature (set_seeds) and callers (wherever it is invoked) are updated to use
the returned/exported rng object so code uses Generator methods instead of
np.random.* globals.
In `@tests/cluster/test_cluster.py`:
- Line 271: Replace the Unicode multiplication sign "×" in the docstring with
the ASCII letter "x" to satisfy the linter and improve compatibility; locate the
docstring in tests/cluster/test_cluster.py (the comment describing "Expected
result: 4 clusters (2 × 2 ...") and change "2 × 2" to "2 x 2" (or any similar
occurrence of "×") so the test docstring uses only ASCII characters.
- Line 79: Remove the leftover debug print statement by deleting the call
print(linkage_matrix) in the test (replace with nothing or, if needed for
debugging, use logging at debug level or an assertion); locate the
print(linkage_matrix) occurrence in the test_cluster test and remove that line
so the test output is clean.
- Line 463: The test unpacks cluster.generate_clusters into labels, reps but
never uses reps; rename the unused variable to _reps to signal intentional
unused and silence linters. Update the three call sites where generate_clusters
is unpacked (the occurrences using variables named reps at lines around the
calls to cluster.generate_clusters) to use _reps instead of reps so tests remain
functionally identical while satisfying the linter.
In `@tests/cluster/test_consensus.py`:
- Around line 166-179: Update the test_small_cluster unit test to also assert
the medoid retention time: after calling consensus._get_cluster_medoids (in
test_small_cluster) add an assertion that retention_times[0] == 10.0 (or use a
small tolerance if floating-point precision is a concern) to confirm the chosen
medoid’s retention time matches the first spectrum’s rts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b58361a-c414-426c-956d-f89fcb61564d
⛔ Files ignored due to path filters (1)
falcon_how_v2.pngis excluded by!**/*.png
📒 Files selected for processing (28)
.gitignoreREADME.mdfalcon/cluster/cluster.pyfalcon/cluster/consensus.pyfalcon/cluster/distance_matrix.pyfalcon/cluster/similarity.pyfalcon/cluster/spectrum.pyfalcon/config.pyfalcon/falcon.pyfalcon/ms_io/mgf_io.pyfalcon/ms_io/ms_io.pyfalcon/ms_io/mzml_io.pyfalcon/ms_io/mzxml_io.pyfalcon/seed.pyfalcon/utils.pysetup.cfgtests/cluster/test_cluster.pytests/cluster/test_consensus.pytests/cluster/test_distance_matrix.pytests/cluster/test_similarity.pytests/cluster/test_spectrum.pytests/conftest.pytests/data/sample.mzmltests/data/sample.mzxmltests/test_config.pytests/test_falcon.pytests/test_ms_io.pytests/test_mzml_mzxml_io.py
💤 Files with no reviewable changes (3)
- falcon/seed.py
- falcon/ms_io/ms_io.py
- falcon/cluster/spectrum.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
falcon/falcon.py (2)
145-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't ignore the current
--precursor_charge_bucketswhen reusing a work dir.When
charges.joblibexists, Line 147 reuses the cached bucket layout unconditionally. A rerun with different bucket settings will silently cluster against stale per-bucket datasets, so the output no longer matches the CLI the user asked for. Compare the cached value toconfig.precursor_charge_bucketsand fail fast or rebuild after clearingwork_dir/spectra.Proposed fix
charge_path = os.path.join(config.work_dir, "spectra", "charges.joblib") if os.path.isfile(charge_path) and not config.overwrite: charge_buckets = joblib.load(charge_path) + if charge_buckets != config.precursor_charge_buckets: + raise ValueError( + "Cached charge buckets do not match the current " + "--precursor_charge_buckets; rerun with --overwrite " + "or a different --work_dir." + ) else: # Recalculate the charge buckets and recreate dataset. charge_buckets = _prepare_spectra( process_spectrum, config.precursor_charge_buckets )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/falcon.py` around lines 145 - 153, The code currently reuses charges.joblib without validating its stored bucket configuration; update the logic around charge_path/charge_buckets to load the cached metadata and compare its stored precursor bucket layout to config.precursor_charge_buckets, and if they differ either (a) delete/clear the work_dir/spectra cache and recompute by calling _prepare_spectra(process_spectrum, config.precursor_charge_buckets) and joblib.dump the new charge_buckets, or (b) raise a clear error indicating bucket mismatch so the caller can remove the spectra directory; ensure the stored metadata in charges.joblib (or alongside it) is read to perform this comparison before reusing the cached charge_buckets.
141-143:⚠️ Potential issue | 🟠 MajorFix
--overwritecleanup for Lance dataset directories (falcon/falcon.pylines 141-143)
os.remove()will raiseIsADirectoryErrorifspectra/contains Lance dataset roots likespectra_charge*.lance(Lance stores datasets under a directory root). Remove directories withshutil.rmtree()instead ofos.remove().Proposed fix
if config.overwrite: for filename in os.listdir(os.path.join(config.work_dir, "spectra")): - os.remove(os.path.join(config.work_dir, "spectra", filename)) + path = os.path.join(config.work_dir, "spectra", filename) + if os.path.isdir(path): + shutil.rmtree(path) + else: + os.remove(path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/falcon.py` around lines 141 - 143, The overwrite cleanup loop in falcon.py uses os.remove on entries inside the spectra directory which fails for Lance dataset directories (e.g., spectra_charge*.lance); update the block where config.overwrite is checked to iterate entries under os.path.join(config.work_dir, "spectra") and for each entry use os.path.isdir to detect directories and call shutil.rmtree to remove them, otherwise call os.remove for files; ensure you import shutil at the top if not already present and preserve existing behavior when entries are regular files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@falcon/falcon.py`:
- Around line 145-153: The code currently reuses charges.joblib without
validating its stored bucket configuration; update the logic around
charge_path/charge_buckets to load the cached metadata and compare its stored
precursor bucket layout to config.precursor_charge_buckets, and if they differ
either (a) delete/clear the work_dir/spectra cache and recompute by calling
_prepare_spectra(process_spectrum, config.precursor_charge_buckets) and
joblib.dump the new charge_buckets, or (b) raise a clear error indicating bucket
mismatch so the caller can remove the spectra directory; ensure the stored
metadata in charges.joblib (or alongside it) is read to perform this comparison
before reusing the cached charge_buckets.
- Around line 141-143: The overwrite cleanup loop in falcon.py uses os.remove on
entries inside the spectra directory which fails for Lance dataset directories
(e.g., spectra_charge*.lance); update the block where config.overwrite is
checked to iterate entries under os.path.join(config.work_dir, "spectra") and
for each entry use os.path.isdir to detect directories and call shutil.rmtree to
remove them, otherwise call os.remove for files; ensure you import shutil at the
top if not already present and preserve existing behavior when entries are
regular files.
to create evenly sized batches
Collaborator
Author
|
How should clusters with known and unknown retention times be handled when |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
falcon/falcon.py (2)
530-567: 💤 Low valueDocstring incorrectly states lock type.
The docstring says
lock : multiprocessing.synchronize.Lockbut the actual type annotation correctly showsthreading.Lock.📝 Suggested fix
- lock : multiprocessing.synchronize.Lock + lock : threading.Lock Lock to synchronize writing to the dataset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/falcon.py` around lines 530 - 567, The docstring for _write_to_dataset incorrectly documents the lock parameter as multiprocessing.synchronize.Lock while the function signature uses threading.Lock; update the docstring's lock parameter description to match the annotation (threading.Lock) and adjust its wording to indicate it's a threading.Lock used to synchronize writes to the Lance dataset (reference: function _write_to_dataset and parameter lock).
385-415: 💤 Low valueType hint for
charge_bucketis incomplete.The parameter can also be
str(e.g.,"other"), but the type hint only showsUnion[int, Tuple[int, ...]]. The caller_write_to_datasetcorrectly types it asUnion[Tuple[int, ...], str].📝 Suggested fix
def _create_lance_dataset( - charge_bucket: Union[int, Tuple[int, ...]], schema: pa.Schema + charge_bucket: Union[Tuple[int, ...], str], schema: pa.Schema ) -> lance.LanceDataset:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/falcon.py` around lines 385 - 415, The type hint for parameter charge_bucket in _create_lance_dataset is missing str; update its annotation from Union[int, Tuple[int, ...]] to Union[Tuple[int, ...], str, int] (or Union[int, Tuple[int, ...], str]) to match callers like _write_to_dataset, and update the corresponding docstring "charge_bucket" type description to include str (e.g., "int | Tuple[int, ...] | str") so bucket_key_to_str usage remains correct.falcon/cluster/consensus.py (1)
571-625: 💤 Low valueDocstring type hints don't match actual parameter types.
The docstring states
bins_peaksandbins_mzarenb.typed.List, but the actual parameter type annotations (lines 574-575) correctly show them asnp.ndarray. This matches how_outlier_rejectionreturns them.📝 Suggested docstring fix
Parameters ---------- bins_indices : List[int] The indices of the non-empty bins. - bins_peaks : nb.typed.List + bins_peaks : np.ndarray The intensities for each bin. - bins_mz : nb.typed.List + bins_mz : np.ndarray The m/z values for each bin🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@falcon/cluster/consensus.py` around lines 571 - 625, Update the _construct_average_spectrum docstring to reflect the actual types used: change bins_peaks and bins_mz from nb.typed.List to np.ndarray (matching the function signature and what _outlier_rejection returns), and ensure the Returns section/type summary aligns with the function signature (e.g., charge is int or np.nan handling) so the documented types match the annotated types in _construct_average_spectrum.tests/conftest.py (1)
115-140: 💤 Low valueUnused loop variable
i.The loop variable
ion line 121 is not used within the loop body. Rename it to_to indicate it's intentionally unused.📝 Suggested fix
- for i in range(n): + for _ in range(n):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 115 - 140, The loop in the pytest fixture factory _make inside mock_spectra uses an unused loop variable i; update the for loop to use a conventional unused name (for example change "for i in range(n):" to "for _ in range(n):") so the intent is clear and linting warnings are removed. Leave the rest of _make and the creation of similarity.SpectrumTuple unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@falcon/cluster/consensus.py`:
- Around line 571-625: Update the _construct_average_spectrum docstring to
reflect the actual types used: change bins_peaks and bins_mz from nb.typed.List
to np.ndarray (matching the function signature and what _outlier_rejection
returns), and ensure the Returns section/type summary aligns with the function
signature (e.g., charge is int or np.nan handling) so the documented types match
the annotated types in _construct_average_spectrum.
In `@falcon/falcon.py`:
- Around line 530-567: The docstring for _write_to_dataset incorrectly documents
the lock parameter as multiprocessing.synchronize.Lock while the function
signature uses threading.Lock; update the docstring's lock parameter description
to match the annotation (threading.Lock) and adjust its wording to indicate it's
a threading.Lock used to synchronize writes to the Lance dataset (reference:
function _write_to_dataset and parameter lock).
- Around line 385-415: The type hint for parameter charge_bucket in
_create_lance_dataset is missing str; update its annotation from Union[int,
Tuple[int, ...]] to Union[Tuple[int, ...], str, int] (or Union[int, Tuple[int,
...], str]) to match callers like _write_to_dataset, and update the
corresponding docstring "charge_bucket" type description to include str (e.g.,
"int | Tuple[int, ...] | str") so bucket_key_to_str usage remains correct.
In `@tests/conftest.py`:
- Around line 115-140: The loop in the pytest fixture factory _make inside
mock_spectra uses an unused loop variable i; update the for loop to use a
conventional unused name (for example change "for i in range(n):" to "for _ in
range(n):") so the intent is clear and linting warnings are removed. Leave the
rest of _make and the creation of similarity.SpectrumTuple unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9b10b92-9d6d-4143-add7-a07fd2474eca
📒 Files selected for processing (15)
falcon/cluster/cluster.pyfalcon/cluster/consensus.pyfalcon/cluster/similarity.pyfalcon/falcon.pyfalcon/ms_io/mzxml_io.pyfalcon/utils.pytests/cluster/test_cluster.pytests/cluster/test_consensus.pytests/cluster/test_distance_matrix.pytests/cluster/test_similarity.pytests/conftest.pytests/test_config.pytests/test_falcon.pytests/test_ms_io.pytests/test_mzml_mzxml_io.py
💤 Files with no reviewable changes (2)
- falcon/cluster/similarity.py
- falcon/ms_io/mzxml_io.py
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/cluster/test_distance_matrix.py
- tests/cluster/test_similarity.py
- falcon/utils.py
- tests/test_mzml_mzxml_io.py
- tests/test_falcon.py
- tests/test_ms_io.py
- tests/cluster/test_cluster.py
- tests/test_config.py
- falcon/cluster/cluster.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_falcon.py (1)
27-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore prior
config.work_dirvalue instead of always deleting it.The fixture teardown at Line 36 unconditionally deletes
work_dir, which can leak state across tests ifconfig.work_dirwas already set before this fixture ran. Save the previous value and restore it afteryield.Proposed fix
`@pytest.fixture` def work_dir(tmp_path): @@ - (tmp_path / "spectra").mkdir() - _global_config.__dict__["work_dir"] = str(tmp_path) - yield tmp_path - del _global_config.__dict__["work_dir"] + (tmp_path / "spectra").mkdir() + had_prev = "work_dir" in _global_config.__dict__ + prev = _global_config.__dict__.get("work_dir") + _global_config.__dict__["work_dir"] = str(tmp_path) + try: + yield tmp_path + finally: + if had_prev: + _global_config.__dict__["work_dir"] = prev + else: + _global_config.__dict__.pop("work_dir", None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_falcon.py` around lines 27 - 36, The fixture work_dir currently unconditionally deletes _global_config.__dict__["work_dir"] in teardown which can remove a previously set value; modify work_dir to capture the prior presence and value before setting (e.g. store a sentinel like _prev_exists and _prev_value), yield as before, and in the teardown restore the previous value if it existed or delete the key only if it did not exist prior—use the same _global_config.__dict__["work_dir"] symbol to locate and revert state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_falcon.py`:
- Around line 27-36: The fixture work_dir currently unconditionally deletes
_global_config.__dict__["work_dir"] in teardown which can remove a previously
set value; modify work_dir to capture the prior presence and value before
setting (e.g. store a sentinel like _prev_exists and _prev_value), yield as
before, and in the teardown restore the previous value if it existed or delete
the key only if it did not exist prior—use the same
_global_config.__dict__["work_dir"] symbol to locate and revert state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6aea1122-465c-47c3-9e80-cb5ebd21c105
📒 Files selected for processing (5)
README.mdfalcon/config.pyfalcon/falcon.pytests/test_config.pytests/test_falcon.py
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- tests/test_config.py
- falcon/config.py
- falcon/falcon.py
Member
Yes, that sounds fine. |
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.
This pull request introduces several significant updates to the Falcon spectrum clustering tool.
Algorithm and Performance Enhancements:
falcon/cluster/distance_matrix.py,falcon/cluster/similarity.py).falcon/cluster/similarity.py).falcon/cluster/consensus.py).Configuration and Usability Improvements:
--consensus_method,--outlier_cutoff_lower,--outlier_cutoff_upper) (falcon/config.py).--precursor_charge_buckets), providing users with greater flexibility (falcon/config.py).falcon/cluster/spectrum.py).Documentation Updates:
README.mdto describe the new clustering pipeline, updated parameters and defaults, and improved installation and usage instructions. The documentation now accurately reflects the hierarchical clustering workflow and new configuration options.Summary by CodeRabbit
New Features
Improvements
Documentation
Tests