Update github actions, more examples added, remove filtering of features based on percentile tic. #51
Conversation
WalkthroughThe changes modify the GitHub Actions workflow for the Python package. A new job titled "Download test files" is introduced that uses Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/python-package.yml (4)
47-50: Enhance Download Step Robustness
The new "Download test files" step useswgetto fetch remote files. Consider adding error handling flags (for example,--failto exit on HTTP errors and--quietfor less verbose output) or aset -eat the start of the run block. This ensures that the workflow fails fast if a download issue occurs.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
54-56: Typo in Step Name
The step is currently labeled "Text package online DIA," which appears to be a typographical error. It likely should read "Test package online DIA" to remain consistent with the naming in the previous step.
47-47: Remove Trailing Space
Line 47 contains trailing spaces. Removing them will help pass YAML linting without warnings.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
57-58: Remove Trailing Spaces
The empty lines (57 and 58) include trailing spaces. Cleaning these up will improve the file's adherence to YAML formatting guidelines.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 57-57: trailing spaces
(trailing-spaces)
[error] 58-58: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/python-package.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/python-package.yml
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 57-57: trailing spaces
(trailing-spaces)
[error] 58-58: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/python-package.yml (1)
51-53: Verify Tool Naming Consistency
There is an inconsistency between the tool names used in the CLI steps. Earlier, the "Test commandline tool" step invokesquantmsutilsc --help, whereas this step callsquantmsutils mzmlstats. Please verify and standardize the CLI tool name to avoid potential issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/python-package.yml (3)
47-50: Improve External File Download Reliability.The "Download test files." step uses
wgetwithout extra options to handle failures or retries. Consider adding flags such as--fail,--quiet, and a retry limit (--tries=3) to ensure that the workflow fails if the download is unsuccessful.
54-56: Correct Typo in Step Name.The step name "Text package online DIA" appears to contain a typo. It should likely be "Test package online DIA" to remain consistent with the naming convention.
57-58: Remove Trailing Spaces.Static analysis indicates there are trailing spaces on these lines. Removing them will improve file cleanliness and maintain compliance with YAML linting standards.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 57-57: trailing spaces
(trailing-spaces)
[error] 58-58: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/python-package.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/python-package.yml
[error] 57-57: trailing spaces
(trailing-spaces)
[error] 58-58: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: build (3.9)
- GitHub Check: build (3.10)
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/python-package.yml (1)
51-53: Verify Command Line Options for TMT Test.The "Test package online TMT" step invokes
quantmsutilsc mzmlstatswith the flags--ms_path,--ms2_file, and--feature_detection. Please verify that the--ms2_fileflag is intended to be used without an argument (i.e., as a boolean flag) or if it needs a file path or value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
quantmsutils/mzml/ms1_feature_finder.py (2)
37-41: Documentation inconsistency: Parameters remain in docstring but not in signatureThe docstring still mentions
min_pticandmax_pticparameters even though they've been removed from the method signature.Parameters ---------- -min_ptic : float, optional - Minimum percentile TIC to include features, by default 0.05 -max_ptic : float, optional - Maximum percentile TIC to include features, by default 0.95 ms_level : int, optional MS level to analyze, by default 1
149-164: Update method docstring to reflect removal of filtering functionalityThe docstring for
_extract_featuresshould be updated to reflect that features are no longer filtered by pTIC.""" -Extract feature information and filter by pTIC. +Extract feature information. Parameters ----------
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
quantmsutils/mzml/ms1_feature_finder.py(3 hunks)quantmsutils/mzml/mzml_statistics.py(2 hunks)quantmsutils/openms.py(1 hunks)tests/test_commands.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- quantmsutils/openms.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (3.10)
- GitHub Check: build (3.9)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
quantmsutils/mzml/ms1_feature_finder.py (2)
31-31: Signature changed to remove percentile TIC parametersThe constructor has been updated to remove the
min_pticandmax_pticparameters. This is part of a larger change that removes filtering based on percentile TIC from the feature detection process.
186-202: Feature filtering based on percentile TIC removedThe conditional filtering of features based on
min_pticandmax_ptichas been removed, so all features are now appended to the feature list regardless of their percentile TIC values. This change will likely result in more features being detected.I recommend validating that this change is intentional and aligns with the team's expectations regarding feature detection behavior.
quantmsutils/mzml/mzml_statistics.py (1)
610-611: Updated to match new MS1FeatureDetector constructor signatureThe instantiation of
MS1FeatureDetectorhas been modified to match the updated constructor signature which no longer includesmin_pticandmax_pticparameters.tests/test_commands.py (2)
207-207: Updated to match new MS1FeatureDetector constructor signatureThe initialization of
MS1FeatureDetectorin the test has been updated to match the modified constructor signature, removing themin_ptic=0.05andmax_ptic=0.95parameters.
207-208:Details
❓ Verification inconclusive
Verify if test coverage is maintained with new feature detection behavior
Since the feature detector now includes all features regardless of percentile TIC values, it would be valuable to verify that the test still adequately validates the feature detection functionality.
🏁 Script executed:
#!/bin/bash # Check if the test still validates features with the updated behavior # Look for any assertions related to feature count or filtering rg -A 5 -B 5 "assert.*feature" tests/test_commands.py # Check expected feature counts or conditions rg -A 3 "len\(.*feature.*\)" tests/test_commands.pyLength of output: 109
Action Required: Verify Test Assertions for Updated Feature Detection
It appears that the current test intests/test_commands.pydoes not include explicit assertions related to feature counts or the filtering behavior after updatingMS1FeatureDetectorto include all features regardless of percentile TIC values. No output was found from the grep searches for assertions involving "feature" or "len(...feature...)". Please manually verify that the test (or additional tests) explicitly checks the detector’s functionality with the new behavior. If necessary, add assertions to confirm the expected feature count and validate that changes in filtering do not lead to unintended side effects.
- Verify that the output (stored in
TMT_MS1_FEAURES) is checked against expectations reflecting the inclusion of all features.- Consider updating or adding tests with explicit assertions on feature counts or other properties relevant to the new behavior.
ypriverol
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
quantmsutils/mzml/ms1_feature_finder.py (2)
34-34: Method signature updated, but docstring needs alignmentThe
__init__method signature has been updated to remove themin_pticandmax_pticparameters, but the docstring (lines 40-43) still references these parameters.- def __init__(self, ms_level: int = 1): + def __init__(self, ms_level: int = 1): """ Initialize the MS1 feature detector. Parameters ---------- - min_ptic : float, optional - Minimum percentile TIC to include features, by default 0.05 - max_ptic : float, optional - Maximum percentile TIC to include features, by default 0.95 ms_level : int, optional MS level to analyze, by default 1 """
152-152: Update method docstring to reflect removal of pTIC filteringThe docstring for
_extract_featuresstill mentions filtering by pTIC, but the code no longer performs this filtering.- Extract feature information and filter by pTIC. + Extract feature information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
quantmsutils/mzml/ms1_feature_finder.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
quantmsutils/mzml/ms1_feature_finder.py (3)
7-13: Docstring correctly reflects the algorithmic improvementsThe updated docstring accurately explains the key improvements including the removal of percentile TIC filtering, which aligns with the actual code changes made in this PR.
144-150: Parameter formatting improvedThe parameter formatting in the
_extract_featuresmethod has been improved for better readability.
189-205: Feature extraction logic simplified by removing pTIC filteringThe code now unconditionally adds all features to the feature list, which aligns with the intention to "remove the filtering of percentile TIC" as stated in the updated documentation. This is a good simplification that allows downstream tools to perform any necessary filtering.
Summary by CodeRabbit
MS1FeatureDetectorclass in tests to use default parameters.MS1FeatureDetectorclass to use default values instead of specified parameters.extract_scan_idfunction.