TMT-LFQ-OpenMS insight integration#29
Open
JKVISION1101 wants to merge 9 commits into
Open
Conversation
This reverts commit 83646ec.
Allows input_TOPP() to designate parameters as flags (present/absent) instead of key-value pairs, persisted to params.json and session_state so run_topp() can correctly build the command line.
Split WorkflowTest configure() into separate LFQ and TMT tool tabs, and add new preprocessing/analysis pages (filtering, normalization, imputation, statistical testing, GO enrichment, clustered heatmap, pathway analysis) built on openms_insight engine functions.
Split WorkflowTest configure() into separate LFQ and TMT tool tabs, and add new preprocessing/analysis pages (filtering, normalization, imputation, statistical testing, GO enrichment, clustered heatmap, pathway analysis) built on openms_insight engine functions.
Split WorkflowTest configure() into separate LFQ and TMT tool tabs, and add new preprocessing/analysis pages (filtering, normalization, imputation, statistical testing, GO enrichment, clustered heatmap, pathway analysis) built on openms_insight engine functions.
📝 WalkthroughWalkthroughChangesDifferential protein analysis
Workflow interfaces
Sequence Diagram(s)sequenceDiagram
participant User
participant StatisticalPage
participant SessionState
participant EnrichmentPage
participant VisualizationPage
User->>StatisticalPage: run statistical analysis
StatisticalPage->>SessionState: store statistics_df
EnrichmentPage->>SessionState: read statistics_df
VisualizationPage->>SessionState: read analysis tables
sequenceDiagram
participant StreamlitUI
participant ParameterManager
participant CommandExecutor
StreamlitUI->>ParameterManager: persist flag metadata
ParameterManager->>CommandExecutor: provide merged parameters
CommandExecutor->>CommandExecutor: emit CLI flags and values
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/workflow/StreamlitUI.py (1)
1695-1713: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude
_flag_paramsfrom the exported method summary.The new internal dictionary is classified as a TOPP tool and appears in user-facing exports. Skip all internal keys while retaining
_defaultsfor the explicit merge below.Proposed fix
for k, v in params.items(): - if k == "_defaults": + if k.startswith("_"): continue🤖 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 `@src/workflow/StreamlitUI.py` around lines 1695 - 1713, Update the parameter classification loop to skip `_flag_params` alongside `_defaults`, preventing this internal dictionary from being added to the exported TOPP summary while preserving the existing `_defaults` merge below.
🧹 Nitpick comments (4)
content/enrichment.py (1)
134-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRender interactive Plotly output through
streamlit_plotly_events.
st.plotly_chartdoes not expose the required selection/click event flow. Route the generated figures throughplotly_eventsand persist the returned interaction state.As per coding guidelines, interactive visualizations must use Plotly and
streamlit_plotly_events.🤖 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 `@content/enrichment.py` around lines 134 - 139, Replace the st.plotly_chart call in the fig/df_go rendering block with streamlit_plotly_events.plotly_events, passing the generated fig and capturing its returned interaction state in the existing session-state flow. Preserve the dataframe rendering and ensure the Plotly figure remains interactive.Source: Coding guidelines
content/statistical.py (1)
96-130: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftIsolate interactive controls and rendering in
@st.fragmentfunctions.Each widget currently triggers a full-page rerun, including abundance loading, conversions, previews, and component initialization.
content/statistical.py#L96-L130: wrap statistical configuration and execution controls in a fragment.content/enrichment.py#L62-L100: fragment the cutoff controls and enrichment trigger.content/results_pca.py#L93-L161: fragment PCA controls and component rendering.content/results_volcano.py#L48-L88: fragment threshold controls and plot rendering.content/results_heatmap.py#L39-L91: fragment variance selection and heatmap rendering.content/results_heatmap_clustered.py#L38-L99: fragment variance selection and clustered rendering.As per coding guidelines,
**/*.pyinteractive UI updates must use@st.fragmentto avoid full-page reloads.🤖 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 `@content/statistical.py` around lines 96 - 130, Wrap the interactive UI sections in `@st.fragment` functions so widget updates do not rerun the full page: content/statistical.py lines 96-130 should contain the statistical configuration and execution controls; content/enrichment.py lines 62-100 the cutoff controls and enrichment trigger; content/results_pca.py lines 93-161 the PCA controls and component rendering; content/results_volcano.py lines 48-88 the threshold controls and plot rendering; content/results_heatmap.py lines 39-91 the variance selection and heatmap rendering; and content/results_heatmap_clustered.py lines 38-99 the variance selection and clustered rendering. Preserve each section’s existing behavior while ensuring all interactive updates are handled within its fragment.Source: Coding guidelines
content/filtering.py (1)
78-124: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftWrap the interactive preprocessing controls in
@st.fragment.These widgets currently rerun each entire page, including abundance loading and large table rendering.
content/filtering.py#L78-L124: move filter controls and execution into a fragment.content/imputation.py#L86-L118: fragment the imputation controls and execution.content/normalization.py#L152-L190: fragment the normalization controls and execution.As per coding guidelines,
**/*.py: Use@st.fragmentdecorator for interactive UI updates without full page reloads. <coding_guidelines>🤖 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 `@content/filtering.py` around lines 78 - 124, Wrap the filtering controls and Apply Filter execution in content/filtering.py (lines 78-124) with an `@st.fragment` function. Apply the same change to the imputation controls and execution in content/imputation.py (lines 86-118), and the normalization controls and execution in content/normalization.py (lines 152-190), preserving each workflow’s existing behavior while limiting widget updates to the fragment.Source: Coding guidelines
pr-397.patch (1)
1-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove this generated patch artifact.
The changes already exist in the source files; retaining this duplicate patch adds noise and will drift from the implementation.
🤖 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 `@pr-397.patch` around lines 1 - 120, Remove the generated patch artifact represented by pr-397.patch from the change set. Keep the existing implementations in CommandExecutor.run_topp and StreamlitUI.input_TOPP unchanged, since those source-file changes already contain the intended functionality.
🤖 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 `@app.py`:
- Around line 1-6: Scope the POLARS_MAX_THREADS override in the app.py startup
initialization to Windows only, applying
os.environ.setdefault("POLARS_MAX_THREADS", "1") when the runtime platform is
Windows. Preserve the ordering so this condition executes before any direct or
transitive Polars import, while leaving non-Windows platforms’ default thread
settings unchanged.
In `@content/filtering.py`:
- Around line 153-154: Invalidate downstream artifacts when replacing
filtered_df: in content/filtering.py lines 153-154, clear imputation,
normalization, and statistics session results. In content/imputation.py lines
50-52, reuse filtered output only when its workspace/input fingerprint matches;
at lines 136-139, clear normalization and statistics after replacing imputed_df.
In content/normalization.py lines 54-62, validate provenance before using cached
upstream outputs; at lines 227-230, clear statistics after replacing
normalized_df.
- Around line 11-15: Resolve the incompatible openms_insight imports by either
pinning the dependency to a fork or revision that provides the analysis
namespace, or migrating the imports to the current supported API. Apply the
chosen fix at content/filtering.py lines 11-15, content/imputation.py lines
10-11, and content/normalization.py lines 9-14 so all three pages import
successfully.
In `@content/results_abundance.py`:
- Around line 63-72: Update the non-LFQ branch around id_col to derive the
identifier from the report schema’s first column instead of hardcoding
"protein". Exclude that derived identifier from sample_cols and preserve it in
display_cols so nonstandard first-column names are not passed to np.log2 or
omitted from the output.
In `@content/results_heatmap.py`:
- Around line 37-49: Update the data source used by the heatmap flow in
content/results_heatmap.py lines 37-49 to select the active preprocessing
checkpoint in the same order as PCA/statistics: normalized, then imputed, then
filtered, then raw, before variance selection. Apply the same checkpoint
selection in content/results_heatmap_clustered.py lines 38-45 before its Z-score
normalization; preserve each page’s existing variance and rendering logic after
using the selected dataframe.
In `@content/statistical.py`:
- Around line 71-95: The sample-column logic in content/statistical.py lines
71-95 and content/results_pca.py lines 66-71 must stop using exclusion lists
that treat metadata columns as samples. Reuse the shared analysis-mode-aware
loader helper, or derive the columns from normalized sample_group_map keys,
consistently in both sites before group counting, variance calculation, and
metadata construction; preserve the existing downstream behavior once the valid
sample set is established.
- Around line 40-69: Bind all shared artifacts to current workspace and upstream
provenance before use: in content/statistical.py lines 40-69, validate
workspace, source-file, parameters, and preprocessing metadata before selecting
normalized_df, imputed_df, or filtered_df; in content/statistical.py lines
130-153, invalidate statistics_df before each new run and store it with
workspace, input, and configuration provenance; in content/results_pca.py lines
34-64, reject preprocessing artifacts from another workspace or abundance
revision; in content/enrichment.py lines 25-46, verify statistics match the
current workspace and abundance inputs; and in content/results_volcano.py lines
22-45, verify statistics provenance before combining with the current workspace
identifier schema.
In `@requirements.txt`:
- Line 149: Update the requirements dependency entries to use the
polars[rtcompat] extra and remove the standalone polars-runtime-compat entry,
while retaining only the existing pinned polars dependency so the runtime
compatibility package stays version-locked.
In `@src/common/results_helpers.py`:
- Around line 397-424: Update get_sample_group_map so every sample column in
pivot_df is present in the returned normalized map, defaulting missing or blank
group_map entries to "Unassigned". Preserve the existing LFQ name normalization
and TMT index-to-column matching, while ensuring unmatched sample columns are
also included with the required default.
In `@src/workflow/CommandExecutor.py`:
- Around line 308-315: Update the regular-parameter handling in CommandExecutor
so list-valued parameters are expanded into separate command arguments rather
than converted to one string representation. Preserve the existing skipping of
empty or None values, option-key emission, and newline splitting for string
values; convert each list element to its own argument using the existing value
formatting behavior.
In `@src/workflow/ParameterManager.py`:
- Around line 84-94: Connect ParamXML boolean discovery through all listed
sites: in src/workflow/ParameterManager.py lines 84-94, expose discovered
boolean paths per tool instance instead of only caching them; in
src/workflow/CommandExecutor.py lines 272-277, merge those instance flags with
persisted explicit flags; in src/workflow/StreamlitUI.py lines 938-948, persist
discovered paths under tool_instance_name; and in src/workflow/StreamlitUI.py
lines 1131-1139, use boolean metadata to render checkboxes and normalize
"true"/"false" values.
In `@src/workflow/StreamlitUI.py`:
- Around line 443-485: Update the selectable-entry handling around _is_match and
the selectable loop to resolve each entry and reject any whose resolved path is
outside the resolved mount_root, including symlinked files and directories.
Perform this validation before rendering or persisting entries, while preserving
the existing directory/file filtering and selection behavior for paths that
remain beneath mount_root.
In `@src/WorkflowTest.py`:
- Around line 576-582: Update the validation in the selected_type handling
before num_plex and channels are created: replace the broad substring/regex
acceptance with an explicit whitelist of supported plex types or enforce a
strict maximum channel count. Ensure invalid or excessively large values trigger
the existing warning and never reach channel widget allocation.
- Around line 612-619: Update the orphan cleanup block in the workflow test to
remove each orphaned channel from both self.params and the parameter manager’s
session-key storage before persistence. Write the cleaned self.params dictionary
directly without reloading or merging the deleted prefixed keys, ensuring
subsequent loads do not reintroduce orphaned TMT-group entries.
---
Outside diff comments:
In `@src/workflow/StreamlitUI.py`:
- Around line 1695-1713: Update the parameter classification loop to skip
`_flag_params` alongside `_defaults`, preventing this internal dictionary from
being added to the exported TOPP summary while preserving the existing
`_defaults` merge below.
---
Nitpick comments:
In `@content/enrichment.py`:
- Around line 134-139: Replace the st.plotly_chart call in the fig/df_go
rendering block with streamlit_plotly_events.plotly_events, passing the
generated fig and capturing its returned interaction state in the existing
session-state flow. Preserve the dataframe rendering and ensure the Plotly
figure remains interactive.
In `@content/filtering.py`:
- Around line 78-124: Wrap the filtering controls and Apply Filter execution in
content/filtering.py (lines 78-124) with an `@st.fragment` function. Apply the
same change to the imputation controls and execution in content/imputation.py
(lines 86-118), and the normalization controls and execution in
content/normalization.py (lines 152-190), preserving each workflow’s existing
behavior while limiting widget updates to the fragment.
In `@content/statistical.py`:
- Around line 96-130: Wrap the interactive UI sections in `@st.fragment` functions
so widget updates do not rerun the full page: content/statistical.py lines
96-130 should contain the statistical configuration and execution controls;
content/enrichment.py lines 62-100 the cutoff controls and enrichment trigger;
content/results_pca.py lines 93-161 the PCA controls and component rendering;
content/results_volcano.py lines 48-88 the threshold controls and plot
rendering; content/results_heatmap.py lines 39-91 the variance selection and
heatmap rendering; and content/results_heatmap_clustered.py lines 38-99 the
variance selection and clustered rendering. Preserve each section’s existing
behavior while ensuring all interactive updates are handled within its fragment.
In `@pr-397.patch`:
- Around line 1-120: Remove the generated patch artifact represented by
pr-397.patch from the change set. Keep the existing implementations in
CommandExecutor.run_topp and StreamlitUI.input_TOPP unchanged, since those
source-file changes already contain the intended functionality.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d64fff3c-3c59-422e-a377-f3ad96fbacee
📒 Files selected for processing (19)
app.pycontent/enrichment.pycontent/filtering.pycontent/imputation.pycontent/normalization.pycontent/results_abundance.pycontent/results_heatmap.pycontent/results_heatmap_clustered.pycontent/results_pca.pycontent/results_proteomicslfq.pycontent/results_volcano.pycontent/statistical.pypr-397.patchrequirements.txtsrc/WorkflowTest.pysrc/common/results_helpers.pysrc/workflow/CommandExecutor.pysrc/workflow/ParameterManager.pysrc/workflow/StreamlitUI.py
Comment on lines
+1
to
+6
| import os | ||
| # Polars' default (CPU-core-count-sized) native thread pool access-violation | ||
| # crashes the whole process on some high-core-count Windows machines when | ||
| # invoked from Streamlit's script-runner thread. Must be set before polars | ||
| # is imported anywhere (including transitively via openms_insight). | ||
| os.environ.setdefault("POLARS_MAX_THREADS", "1") |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== app.py ==\n'
nl -ba app.py | sed -n '1,120p'
printf '\n== search POLARS_MAX_THREADS / polars imports ==\n'
rg -n --hidden -S 'POLARS_MAX_THREADS|import polars|from polars|thread_pool_size|thread pool' .
printf '\n== files mentioning openms_insight ==\n'
rg -n --hidden -S 'openms_insight' .Repository: OpenMS/quantms-web
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== app.py ==\n'
sed -n '1,120p' app.py | cat -n
printf '\n== search POLARS_MAX_THREADS / polars imports ==\n'
grep -RIn --exclude-dir=.git --exclude-dir=.venv --exclude-dir=node_modules \
-e 'POLARS_MAX_THREADS' -e 'import polars' -e 'from polars' -e 'thread_pool_size' -e 'thread pool' .
printf '\n== files mentioning openms_insight ==\n'
grep -RIn --exclude-dir=.git --exclude-dir=.venv --exclude-dir=node_modules \
-e 'openms_insight' .Repository: OpenMS/quantms-web
Length of output: 6090
Scope POLARS_MAX_THREADS to Windows.
app.py:1-6 sets this override for every platform, so Linux/macOS deployments lose Polars’ default parallelism too.
Proposed fix
import os
+import sys
-if os.environ.setdefault("POLARS_MAX_THREADS", "1")
+if sys.platform == "win32":
+ os.environ.setdefault("POLARS_MAX_THREADS", "1")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import os | |
| # Polars' default (CPU-core-count-sized) native thread pool access-violation | |
| # crashes the whole process on some high-core-count Windows machines when | |
| # invoked from Streamlit's script-runner thread. Must be set before polars | |
| # is imported anywhere (including transitively via openms_insight). | |
| os.environ.setdefault("POLARS_MAX_THREADS", "1") | |
| import os | |
| import sys | |
| # Polars' default (CPU-core-count-sized) native thread pool access-violation | |
| # crashes the whole process on some high-core-count Windows machines when | |
| # invoked from Streamlit's script-runner thread. Must be set before polars | |
| # is imported anywhere (including transitively via openms_insight). | |
| if sys.platform == "win32": | |
| os.environ.setdefault("POLARS_MAX_THREADS", "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 `@app.py` around lines 1 - 6, Scope the POLARS_MAX_THREADS override in the
app.py startup initialization to Windows only, applying
os.environ.setdefault("POLARS_MAX_THREADS", "1") when the runtime platform is
Windows. Preserve the ordering so this condition executes before any direct or
transitive Polars import, while leaving non-Windows platforms’ default thread
settings unchanged.
Comment on lines
+11
to
+15
| from openms_insight.analysis.filter import ( | ||
| filter_low_abundance, | ||
| filter_low_repeatability, | ||
| filter_low_variance, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== repo files =='
git ls-files 'content/*.py' 'src/common/common.py' 'pyproject.toml' 'requirements*.txt' 'setup.py' 'setup.cfg' 'environment*.yml' | sed 's#^`#-` #'
printf '\n%s\n' '== relevant imports/usages =='
for f in content/filtering.py content/imputation.py content/normalization.py; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n "openms_insight\.analysis|page_setup\(|`@st`\.fragment|executor\.run_topp|streamlit_plotly_events|plotly" "$f" || true
fi
done
printf '\n%s\n' '== package availability probe =='
python3 - <<'PY'
import importlib.util
mods = [
"openms_insight",
"openms_insight.analysis",
"openms_insight.analysis.filter",
"openms_insight.analysis.imputation",
"openms_insight.analysis.normalization",
]
for m in mods:
spec = importlib.util.find_spec(m)
print(f"{m}: {'FOUND' if spec else 'MISSING'}")
PY
printf '\n%s\n' '== tree around openms_insight if present in repo =='
fd -t f 'openms_insight*' . || trueRepository: OpenMS/quantms-web
Length of output: 1421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== requirements.txt =='
cat -n requirements.txt
printf '\n%s\n' '== openms_insight references in repo =='
rg -n "openms_insight" .
printf '\n%s\n' '== top of affected files =='
for f in content/filtering.py content/imputation.py content/normalization.py; do
echo "--- $f"
sed -n '1,40p' "$f"
doneRepository: OpenMS/quantms-web
Length of output: 10510
🌐 Web query:
openms-insight 0.1.13 openms_insight.analysis package
💡 Result:
OpenMS-Insight is a Python package designed to provide interactive visualization components for mass spectrometry data within Streamlit applications, leveraging Vue.js for the frontend [1][2]. While the package structure includes core visualization classes like Table, LinePlot, Heatmap, and VolcanoPlot, there is no public-facing module explicitly named openms_insight.analysis in the 0.1.13 version or subsequent releases [1][3]. The package's primary functionality is focused on data visualization and component-based interactivity, such as cross-component selection linking, memory-efficient preprocessing, and disk-based caching [1][4]. Developers extending the package add new components by implementing specific abstract methods—such as _preprocess, _prepare_vue_data, and _get_cache_config—within a component class, rather than using a dedicated analysis package [5]. If you are looking for mass spectrometry analysis tools, you should refer to the core pyOpenMS library, which contains extensive modules for tasks such as chromatographic analysis, peak picking, and signal processing [6]. OpenMS-Insight is specifically intended for the visualization layer of a Streamlit-based data science workflow, not as a backend for raw data analysis algorithms [2][4].
Citations:
- 1: https://pypi.org/project/openms-insight/0.1.13/
- 2: https://github.com/t0mdavid-m/OpenMS-Insight/blob/main/README.md
- 3: https://pypi.org/project/openms-insight/0.1.14/
- 4: https://github.com/hjn0415a/OpenMS-Insight
- 5: https://github.com/t0mdavid-m/OpenMS-Insight/blob/main/CONTRIBUTING.md
- 6: https://pyopenms.readthedocs.io/en/latest/user_guide/chromatographic_analysis.html
Pin a dependency or rewrite these imports
content/filtering.pycontent/imputation.pycontent/normalization.py
openms-insight>=0.1.13 does not expose openms_insight.analysis.*, so these pages fail on import unless the dependency is switched to a fork/revision that provides that namespace or the code is moved to the current API.
📍 Affects 3 files
content/filtering.py#L11-L15(this comment)content/imputation.py#L10-L11content/normalization.py#L9-L14
🤖 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 `@content/filtering.py` around lines 11 - 15, Resolve the incompatible
openms_insight imports by either pinning the dependency to a fork or revision
that provides the analysis namespace, or migrating the imports to the current
supported API. Apply the chosen fix at content/filtering.py lines 11-15,
content/imputation.py lines 10-11, and content/normalization.py lines 9-14 so
all three pages import successfully.
Comment on lines
+153
to
+154
| filtered_df = strip_stat_columns(filtered_lazy.collect().to_pandas()) | ||
| st.session_state["filtered_df"] = filtered_df |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Invalidate or version downstream pipeline artifacts.
The session keys have no workspace/input fingerprint, and recomputing an upstream stage leaves downstream results intact. Users can therefore run statistics against outputs from an older filter, workspace, or abundance dataset.
content/filtering.py#L153-L154: clear imputation, normalization, and statistics after replacingfiltered_df.content/imputation.py#L50-L52: only reuse filtering output when its workspace/input fingerprint matches.content/imputation.py#L136-L139: clear normalization and statistics after replacingimputed_df.content/normalization.py#L54-L62: validate provenance before selecting cached upstream outputs.content/normalization.py#L227-L230: clear statistics after replacingnormalized_df.
📍 Affects 3 files
content/filtering.py#L153-L154(this comment)content/imputation.py#L50-L52content/imputation.py#L136-L139content/normalization.py#L54-L62content/normalization.py#L227-L230
🤖 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 `@content/filtering.py` around lines 153 - 154, Invalidate downstream artifacts
when replacing filtered_df: in content/filtering.py lines 153-154, clear
imputation, normalization, and statistics session results. In
content/imputation.py lines 50-52, reuse filtered output only when its
workspace/input fingerprint matches; at lines 136-139, clear normalization and
statistics after replacing imputed_df. In content/normalization.py lines 54-62,
validate provenance before using cached upstream outputs; at lines 227-230,
clear statistics after replacing normalized_df.
Comment on lines
63
to
+72
| else: | ||
| # Handle non-LFQ mode columns (Log2-transformed Intensity) | ||
| id_col = "protein" | ||
| exclude_cols = [id_col, "log2FC", "p-value", "p-adj", "n_proteins", "n_peptides", "protein_score"] | ||
| exclude_cols = [id_col, "n_proteins", "n_peptides", "protein_score"] | ||
| sample_cols = [c for c in pivot_df.columns if c not in exclude_cols and "ratio" not in c.lower()] | ||
|
|
||
| pivot_df["Intensity"] = pivot_df[sample_cols].apply( | ||
| lambda row: [np.log2(v + 1) for v in row], axis=1 | ||
| ) | ||
| display_cols = [id_col, "log2FC", "p-value", "Intensity"] + sample_cols | ||
| display_cols = [id_col, "Intensity"] + sample_cols |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Derive the TMT identifier from the report schema.
The shared loader supports any first-column identifier, but this renderer hardcodes protein. Other valid names become sample columns and reach np.log2, causing a type error and omitting the identifier from the table.
Proposed fix
- id_col = "protein"
+ id_col = pivot_df.columns[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| else: | |
| # Handle non-LFQ mode columns (Log2-transformed Intensity) | |
| id_col = "protein" | |
| exclude_cols = [id_col, "log2FC", "p-value", "p-adj", "n_proteins", "n_peptides", "protein_score"] | |
| exclude_cols = [id_col, "n_proteins", "n_peptides", "protein_score"] | |
| sample_cols = [c for c in pivot_df.columns if c not in exclude_cols and "ratio" not in c.lower()] | |
| pivot_df["Intensity"] = pivot_df[sample_cols].apply( | |
| lambda row: [np.log2(v + 1) for v in row], axis=1 | |
| ) | |
| display_cols = [id_col, "log2FC", "p-value", "Intensity"] + sample_cols | |
| display_cols = [id_col, "Intensity"] + sample_cols | |
| else: | |
| # Handle non-LFQ mode columns (Log2-transformed Intensity) | |
| id_col = pivot_df.columns[0] | |
| exclude_cols = [id_col, "n_proteins", "n_peptides", "protein_score"] | |
| sample_cols = [c for c in pivot_df.columns if c not in exclude_cols and "ratio" not in c.lower()] | |
| pivot_df["Intensity"] = pivot_df[sample_cols].apply( | |
| lambda row: [np.log2(v + 1) for v in row], axis=1 | |
| ) | |
| display_cols = [id_col, "Intensity"] + sample_cols |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 72-72: Consider [id_col, "Intensity", *sample_cols] instead of concatenation
Replace with [id_col, "Intensity", *sample_cols]
(RUF005)
🤖 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 `@content/results_abundance.py` around lines 63 - 72, Update the non-LFQ branch
around id_col to derive the identifier from the report schema’s first column
instead of hardcoding "protein". Exclude that derived identifier from
sample_cols and preserve it in display_cols so nonstandard first-column names
are not passed to np.log2 or omitted from the output.
Comment on lines
+37
to
+49
| sample_cols = expr_df.columns.tolist() | ||
|
|
||
| # UI settings (number of top variance proteins) | ||
| top_n = st.slider("Number of proteins (Highest Variance)", 20, 200, 50, key="heatmap_top_n") | ||
|
|
||
| # Process data (variance selection -> Z-score normalization) | ||
| var_series = expr_df.var(axis=1) | ||
| top_proteins = var_series.sort_values(ascending=False).head(top_n).index | ||
| heatmap_df = expr_df.loc[top_proteins] | ||
|
|
||
| # Compute Z-scores and clean missing/invalid values | ||
| heatmap_z = heatmap_df.sub(heatmap_df.mean(axis=1), axis=0).div(heatmap_df.std(axis=1), axis=0) | ||
| heatmap_z = heatmap_z.replace([np.inf, -np.inf], np.nan).dropna() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Render heatmaps from the active preprocessing checkpoint.
Both pages always use the original expr_df, so filtering, imputation, and normalization have no effect on either heatmap.
content/results_heatmap.py#L37-L49: select normalized → imputed → filtered → raw data consistently with PCA/statistics before variance selection.content/results_heatmap_clustered.py#L38-L45: apply the same checkpoint selection before Z-score normalization.
📍 Affects 2 files
content/results_heatmap.py#L37-L49(this comment)content/results_heatmap_clustered.py#L38-L45
🤖 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 `@content/results_heatmap.py` around lines 37 - 49, Update the data source used
by the heatmap flow in content/results_heatmap.py lines 37-49 to select the
active preprocessing checkpoint in the same order as PCA/statistics: normalized,
then imputed, then filtered, then raw, before variance selection. Apply the same
checkpoint selection in content/results_heatmap_clustered.py lines 38-45 before
its Z-score normalization; preserve each page’s existing variance and rendering
logic after using the selected dataframe.
Comment on lines
+308
to
315
| # Regular parameter: skip empty/None, append value otherwise | ||
| if v == "" or v is None: | ||
| continue | ||
| command += [f"-{k}"] | ||
| if isinstance(v, str) and "\n" in v: | ||
| command += v.split("\n") | ||
| elif isinstance(v, bool): | ||
| command += [str(v).lower()] | ||
| else: | ||
| command += [str(v)] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expand list parameters instead of stringifying them.
A merged value such as ["A", "B"] becomes the single argument "['A', 'B']". This breaks imported/default list-valued TOPP parameters.
Proposed fix
if isinstance(v, str) and "\n" in v:
command += v.split("\n")
+ elif isinstance(v, list):
+ command += [str(item) for item in v]
else:
command += [str(v)]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Regular parameter: skip empty/None, append value otherwise | |
| if v == "" or v is None: | |
| continue | |
| command += [f"-{k}"] | |
| if isinstance(v, str) and "\n" in v: | |
| command += v.split("\n") | |
| elif isinstance(v, bool): | |
| command += [str(v).lower()] | |
| else: | |
| command += [str(v)] | |
| # Regular parameter: skip empty/None, append value otherwise | |
| if v == "" or v is None: | |
| continue | |
| command += [f"-{k}"] | |
| if isinstance(v, str) and "\n" in v: | |
| command += v.split("\n") | |
| elif isinstance(v, list): | |
| command += [str(item) for item in v] | |
| else: | |
| command += [str(v)] |
🤖 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 `@src/workflow/CommandExecutor.py` around lines 308 - 315, Update the
regular-parameter handling in CommandExecutor so list-valued parameters are
expanded into separate command arguments rather than converted to one string
representation. Preserve the existing skipping of empty or None values,
option-key emission, and newline splitting for string values; convert each list
element to its own argument using the existing value formatting behavior.
Comment on lines
+84
to
+94
| def _merge_bool_params_from_ini(self, tool: str) -> None: | ||
| """Load tool.ini (XML) and merge type=bool parameter paths into session_state.""" | ||
| ini_path = Path(self.ini_dir, f"{tool}.ini") | ||
| if not ini_path.exists(): | ||
| return | ||
| try: | ||
| sk = self.bool_pairs_session_key() | ||
| if sk not in st.session_state: | ||
| st.session_state[sk] = set() | ||
| for short in bool_param_paths_from_param_xml_ini(ini_path, tool): | ||
| st.session_state[sk].add((tool, short)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Connect ParamXML boolean discovery to rendering and execution.
The discovered boolean set currently terminates in an unused session cache, so automatic checkbox rendering and presence-only CLI serialization still depend on manually maintained lists.
src/workflow/ParameterManager.py#L84-L94: expose discovered paths for each tool instance.src/workflow/CommandExecutor.py#L272-L277: merge discovered instance flags with persisted explicit flags.src/workflow/StreamlitUI.py#L938-L948: persist the discovered paths undertool_instance_name.src/workflow/StreamlitUI.py#L1131-L1139: choose checkboxes from boolean metadata and normalize"true"/"false"values.
📍 Affects 3 files
src/workflow/ParameterManager.py#L84-L94(this comment)src/workflow/CommandExecutor.py#L272-L277src/workflow/StreamlitUI.py#L938-L948src/workflow/StreamlitUI.py#L1131-L1139
🤖 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 `@src/workflow/ParameterManager.py` around lines 84 - 94, Connect ParamXML
boolean discovery through all listed sites: in src/workflow/ParameterManager.py
lines 84-94, expose discovered boolean paths per tool instance instead of only
caching them; in src/workflow/CommandExecutor.py lines 272-277, merge those
instance flags with persisted explicit flags; in src/workflow/StreamlitUI.py
lines 938-948, persist discovered paths under tool_instance_name; and in
src/workflow/StreamlitUI.py lines 1131-1139, use boolean metadata to render
checkboxes and normalize "true"/"false" values.
Comment on lines
+443
to
+485
| try: | ||
| entries = sorted( | ||
| (p for p in cwd.iterdir() if not p.name.startswith(".")), | ||
| key=lambda p: (not p.is_dir(), p.name.lower()), | ||
| ) | ||
| except PermissionError: | ||
| st.error(f"Permission denied reading `{cwd}`.") | ||
| return | ||
|
|
||
| def _is_match(p: Path) -> bool: | ||
| return any(p.name.endswith(f".{ft}") for ft in file_types) | ||
|
|
||
| subdirs = [p for p in entries if p.is_dir() and not _is_match(p)] | ||
| bundled = [p for p in entries if p.is_dir() and _is_match(p)] | ||
| files = [p for p in entries if p.is_file() and _is_match(p)] | ||
|
|
||
| for d in subdirs: | ||
| indent, body = st.columns([1, 60], vertical_alignment="center") | ||
| if body.button( | ||
| f"📂 {d.name}/", | ||
| key=f"mounted_dir_{key}_{d.name}", | ||
| type="tertiary", | ||
| ): | ||
| st.session_state[sess_cwd_key] = str(d) | ||
| st.rerun(scope="fragment") | ||
|
|
||
| selectable = bundled + files | ||
| selected_paths: List[str] = [] | ||
| for f in selectable: | ||
| cb_key = f"mounted_pick_{key}_{f}" | ||
| size_label = "" | ||
| if f.is_file(): | ||
| try: | ||
| size_mb = f.stat().st_size / (1024 * 1024) | ||
| size_label = f" · {size_mb:.1f} MB" | ||
| except OSError: | ||
| pass | ||
| icon = "🗂️" if f.is_dir() else "📄" | ||
| if st.checkbox( | ||
| f"{icon} {f.name}{size_label}", | ||
| key=cb_key, | ||
| ): | ||
| selected_paths.append(str(f)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject symlink targets outside the mounted root.
A symlinked file under mount_root passes is_file() and is persisted without resolving its target, allowing the browser to reference files outside the configured mount. Resolve every selectable entry and verify it remains beneath mount_root before rendering or saving it.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 460-460: Unpacked variable indent is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 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 `@src/workflow/StreamlitUI.py` around lines 443 - 485, Update the
selectable-entry handling around _is_match and the selectable loop to resolve
each entry and reject any whose resolved path is outside the resolved
mount_root, including symlinked files and directories. Perform this validation
before rendering or persisting entries, while preserving the existing
directory/file filtering and selection behavior for paths that remain beneath
mount_root.
Comment on lines
+576
to
+582
| m = re.search(r'\d+', selected_type) | ||
| is_supported_type = any(label in selected_type for label in ["tmt", "itraq"]) | ||
| if not m or not is_supported_type: | ||
| st.warning("Please select a supported isobaric type in the IsobaricAnalyzer tab first.") | ||
| else: | ||
| num_plex = int(m.group()) | ||
| channels = [f"sample{i+1}" for i in range(num_plex)] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Whitelist plex types before allocating channel widgets.
The substring/regex check accepts values such as tmt100000000plex; an invalid imported parameter can generate an unbounded number of widgets and hang the session. Validate against supported types, or enforce a strict channel limit.
🤖 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 `@src/WorkflowTest.py` around lines 576 - 582, Update the validation in the
selected_type handling before num_plex and channels are created: replace the
broad substring/regex acceptance with an explicit whitelist of supported plex
types or enforce a strict maximum channel count. Ensure invalid or excessively
large values trigger the existing warning and never reach channel widget
allocation.
Comment on lines
+612
to
+619
| # Remove orphaned params from a previously selected larger plex | ||
| self.params = self.parameter_manager.get_parameters_from_json() | ||
| valid_keys = {f"TMT-group-{ch}" for ch in channels} | ||
| orphaned = [k for k in self.params if k.startswith("TMT-group-") and k not in valid_keys] | ||
| if orphaned: | ||
| for k in orphaned: | ||
| del self.params[k] | ||
| self.parameter_manager.save_parameters() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist orphan removal instead of reloading the deleted values.
self.params is modified, but save_parameters() reloads params.json and merges all prefixed session keys, so the orphaned channels remain or are reintroduced. Remove their session keys and write the cleaned dictionary directly.
Proposed fix
if orphaned:
for k in orphaned:
del self.params[k]
- self.parameter_manager.save_parameters()
+ st.session_state.pop(
+ f"{self.parameter_manager.param_prefix}{k}",
+ None,
+ )
+ with open(
+ self.parameter_manager.params_file,
+ "w",
+ encoding="utf-8",
+ ) as params_file:
+ json.dump(self.params, params_file, indent=4)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Remove orphaned params from a previously selected larger plex | |
| self.params = self.parameter_manager.get_parameters_from_json() | |
| valid_keys = {f"TMT-group-{ch}" for ch in channels} | |
| orphaned = [k for k in self.params if k.startswith("TMT-group-") and k not in valid_keys] | |
| if orphaned: | |
| for k in orphaned: | |
| del self.params[k] | |
| self.parameter_manager.save_parameters() | |
| # Remove orphaned params from a previously selected larger plex | |
| self.params = self.parameter_manager.get_parameters_from_json() | |
| valid_keys = {f"TMT-group-{ch}" for ch in channels} | |
| orphaned = [k for k in self.params if k.startswith("TMT-group-") and k not in valid_keys] | |
| if orphaned: | |
| for k in orphaned: | |
| del self.params[k] | |
| st.session_state.pop( | |
| f"{self.parameter_manager.param_prefix}{k}", | |
| None, | |
| ) | |
| with open( | |
| self.parameter_manager.params_file, | |
| "w", | |
| encoding="utf-8", | |
| ) as params_file: | |
| json.dump(self.params, params_file, indent=4) |
🤖 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 `@src/WorkflowTest.py` around lines 612 - 619, Update the orphan cleanup block
in the workflow test to remove each orphaned channel from both self.params and
the parameter manager’s session-key storage before persistence. Write the
cleaned self.params dictionary directly without reloading or merging the deleted
prefixed keys, ensuring subsequent loads do not reintroduce orphaned TMT-group
entries.
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.
Re-integrate LFQ and TMT Workflow Modes
Summary
Re-introduces Label-Free Quantification (LFQ) and Tandem Mass Tag (TMT) proteomics workflows with improved UI reactivity and parameter handling.
Key Features
Dual Workflow Modes
Reactive UI Components
reactive: boolparameter to conditionally re-render parent fragmentsBoolean CLI Flags
-parameter_name(not-parameter_name true)PeptideIndexing:IL_equivalent,mass_recalibration,delete_unreferenced_peptide_hitsData Processing
Visualization Integration
New Analysis Pages
Files Changed
src/WorkflowTest.py- Dual workflow implementationsrc/workflow/StreamlitUI.py- Reactive mode supportsrc/workflow/ParameterManager.py- Boolean flag handlingsrc/workflow/CommandExecutor.py- CLI flag formattingsrc/common/results_helpers.py- LFQ/TMT data parsingcontent/- New analysis pagesTesting
Summary by CodeRabbit
New Features
Bug Fixes