Add FLASHIda Support #8
Conversation
WalkthroughA new FLASHIda workflow feature was introduced, including its configuration and execution logic, a Streamlit-based UI page, and dynamic page inclusion based on a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StreamlitApp
participant settings.json
participant FLASHIdaWorkflowUI
participant IdaWorkflow
participant IdaSimulatorWorkflow
participant FLASHIdaViewer
participant FLASHIdaSimulatorViewer
participant parseIda
User->>StreamlitApp: Launch app
StreamlitApp->>settings.json: Read enable_flashida
alt enable_flashida == true
StreamlitApp->>FLASHIdaWorkflowUI: Add FLASHIda page group with Workflow, Simulator, Viewer
end
User->>FLASHIdaWorkflowUI: Navigate to FLASHIda Workflow page
FLASHIdaWorkflowUI->>IdaWorkflow: Initialize workflow
User->>FLASHIdaWorkflowUI: Configure parameters (Configure tab)
User->>FLASHIdaWorkflowUI: Start workflow (Run tab)
FLASHIdaWorkflowUI->>IdaWorkflow: Execute workflow logic (monitor files, run executables)
User->>FLASHIdaWorkflowUI: Navigate to FLASHIda Simulator page
FLASHIdaWorkflowUI->>IdaSimulatorWorkflow: Initialize simulator workflow
User->>FLASHIdaWorkflowUI: Upload file (Upload tab)
User->>FLASHIdaWorkflowUI: Configure parameters (Configure tab)
User->>FLASHIdaWorkflowUI: Run simulation (Run tab)
FLASHIdaWorkflowUI->>IdaSimulatorWorkflow: Execute simulation logic (convert, generate method, run executable)
User->>FLASHIdaWorkflowUI: Navigate to FLASHIda Viewer page
FLASHIdaWorkflowUI->>FLASHIdaViewer: Initialize viewer UI
FLASHIdaViewer->>FLASHIdaViewer: Load experiment results
User->>FLASHIdaViewer: Select experiments and layout
FLASHIdaViewer->>FLASHIdaViewer: Render experiment data grids
User->>FLASHIdaWorkflowUI: Navigate to FLASHIda Simulator Viewer page
FLASHIdaWorkflowUI->>FLASHIdaSimulatorViewer: Initialize simulator viewer UI
FLASHIdaSimulatorViewer->>FLASHIdaSimulatorViewer: Load simulation results
User->>FLASHIdaSimulatorViewer: Select experiments and layout
FLASHIdaSimulatorViewer->>FLASHIdaSimulatorViewer: Render simulation data grids
FLASHIdaSimulatorWorkflow->>parseIda: Parse simulation results
parseIda->>FLASHIdaSimulatorWorkflow: Store parsed heatmaps and data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (2)
content/FLASHIda/FLASHIdaWorkflow.py (2)
11-11: Consider adding a brief description.The title is clear, but consider adding a short description below the title to help users understand the purpose of the FLASHIda workflow.
st.title('FLASHIda - Intelligent Data Acquisition') +st.write("Configure and run intelligent data acquisition workflows for mass spectrometry experiments.")
15-20: Consider error handling for workflow initialization.The workflow initialization and UI rendering could benefit from basic error handling in case the IdaWorkflow fails to initialize properly.
t = st.tabs(["⚙️ **Configure**", "🚀 **Run**"]) +try: with t[0]: wf.show_parameter_section() with t[1]: wf.show_execution_section() +except Exception as e: + st.error(f"Error initializing FLASHIda workflow: {str(e)}") + st.info("Please check your installation and configuration settings.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app.py(2 hunks)content/FLASHIda/FLASHIdaWorkflow.py(1 hunks)settings.json(1 hunks)src/Workflow.py(2 hunks)src/workflow/StreamlitUI.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
content/FLASHIda/FLASHIdaWorkflow.py (3)
src/Workflow.py (1)
IdaWorkflow(414-531)src/common/common.py (1)
page_setup(91-228)src/workflow/WorkflowManager.py (2)
show_parameter_section(69-73)show_execution_section(75-79)
🪛 Ruff (0.8.2)
app.py
3-3: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
src/Workflow.py
448-448: f-string without any placeholders
Remove extraneous f prefix
(F541)
456-456: f-string without any placeholders
Remove extraneous f prefix
(F541)
467-467: f-string without any placeholders
Remove extraneous f prefix
(F541)
492-492: Loop control variable method not used within loop body
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (9)
settings.json (1)
15-16: Settings addition looks good.The new
enable_flashidasetting is properly added to the configuration JSON with the correct formatting and indentation.app.py (1)
37-43: Well-implemented conditional page inclusion.The FLASHIda page group is correctly added to the navigation when enabled in settings. This conditional page inclusion approach is clean and follows good practices for feature toggles.
src/workflow/StreamlitUI.py (1)
125-128: Added explicit handling for empty lists.Good defensive programming improvement to handle empty lists explicitly rather than relying on implicit behavior. This prevents potential issues when
tool_textis empty.content/FLASHIda/FLASHIdaWorkflow.py (1)
1-20: New FLASHIda workflow UI looks clean and follows application patterns.The implementation follows the established patterns for workflow pages in the application. It correctly:
- Imports the necessary modules
- Sets up the page
- Creates the IdaWorkflow instance
- Structures the UI with appropriate tabs
The code is concise and well-organized.
src/Workflow.py (5)
442-451: Remove the pointlessf-prefixes in log stringsThe strings in these
self.logger.log()calls don’t contain any interpolation placeholders, yet they’re declared as f-strings.
Besides triggering Ruff’sF541warning, the extrafslightly degrades readability and is unnecessary.- self.logger.log(f'Found FLASHIda executable!') + self.logger.log('Found FLASHIda executable!') … - self.logger.log(f'{flashida_path} is not a file.') + self.logger.log(str(flashida_path) + ' is not a file.')(Same applies to the similar log statements further down.)
[ suggest_nitpick ]🧰 Tools
🪛 Ruff (0.8.2)
448-448: f-string without any placeholders
Remove extraneous
fprefix(F541)
454-468: Same issue: strip the redundantf-prefixesThe two blocks that validate the method and raw-file folders repeat the “empty f-string” pattern.
- self.logger.log(f'Found methods folder!') + self.logger.log('Found methods folder!') … - self.logger.log(f'Found raw file folder!') + self.logger.log('Found raw file folder!')Removing them keeps the style consistent and silences Ruff.
[ suggest_nitpick ]🧰 Tools
🪛 Ruff (0.8.2)
456-456: f-string without any placeholders
Remove extraneous
fprefix(F541)
467-467: f-string without any placeholders
Remove extraneous
fprefix(F541)
505-511: Critical bug:method_pathis overwritten & wrong paths passed to FLASHIda
method_pathis first set to the expected XML file, then immediately replaced by the raw-file path (Path(raw_folder_path, raw)).
As a result:
• The subsequent existence check tests the raw file, not the method file.
•-mreceives the wrong argument, and-rreceives only the filename, not its full path.
rawis just the filename, so-rcan’t be resolved unless the working directory matchesraw_folder_path.- method_path = Path(methods_folder_path, f'{method}.xml') - method_path = Path(raw_folder_path, raw) - if method_path.is_file(): - self.logger.log(f'Found method \'{method_path}\'!') - self.executor.run_command( - [flashida_path, '-m', method_path, '-r', raw] - ) + method_path = Path(methods_folder_path, f'{method}.xml') + raw_path = Path(raw_folder_path, raw) + + if method_path.is_file(): + self.logger.log(f'Found method \'{method_path}\'!') + self.executor.run_command( + [str(flashida_path), '-m', str(method_path), '-r', str(raw_path)] + )This fix preserves both paths correctly and avoids accidental mis-routing of parameters.
[ raise_critical_issue ]
486-498: Infinitewhile Trueloop will block Streamlit session
execution()runs inside a Streamlit callback. An endless loop withsleep(1):• Blocks the server thread, freezing the UI.
• Prevents the user from stopping the workflow gracefully.
• Continuously scans the directory (O(N) listdir every second), which is wasteful for large folders.Consider moving the listener to a background thread/process or using a filesystem watcher (
watchdog), and give the user a “Stop” button or timeout.[ suggest_essential_refactor ][ offer_architecture_advice ]
🧰 Tools
🪛 Ruff (0.8.2)
492-492: Loop control variable
methodnot used within loop body(B007)
520-531: Minor optimisation: pre-compile the regex & iterate withPath.iterdir()
re.search()recompiles the pattern for every file. Compile it once, and factor out theis_filecheck withPath.iterdir()for clearer, faster code.pattern = re.compile(r'^FLASHIda_([^_]+)(?:_[^.]+)?\.raw$') for p in Path(raw_path).iterdir(): if not p.is_file(): continue m = pattern.match(p.name) if m: raws.append(p.name) methods.append(m.group(1))[ suggest_optional_refactor ]
| import json | ||
| # For some reason the windows version only works if this is imported here | ||
| import pyopenms | ||
|
|
||
| import streamlit as st | ||
|
|
||
| from pathlib import Path | ||
|
|
||
|
|
There was a problem hiding this comment.
Import order change and unused import.
The import order has been reorganized, but there's an unused import that should be addressed.
-import json
-# For some reason the windows version only works if this is imported here
-import pyopenms
-
-import streamlit as st
-
-from pathlib import Path
-
-
+import json
+# For some reason the windows version only works if this is imported here
+import pyopenms # noqa: F401
+
+import streamlit as st
+from pathlib import PathConsider adding a # noqa: F401 comment to the pyopenms import to silence the linter warning, since there's a specific comment indicating it's required for Windows compatibility.
📝 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.
| import json | |
| # For some reason the windows version only works if this is imported here | |
| import pyopenms | |
| import streamlit as st | |
| from pathlib import Path | |
| import json | |
| # For some reason the windows version only works if this is imported here | |
| import pyopenms # noqa: F401 | |
| import streamlit as st | |
| from pathlib import Path |
🧰 Tools
🪛 Ruff (0.8.2)
3-3: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/Workflow.py (6)
440-456: Good validation of primary executable.The code properly validates that the primary FLASHIda executable exists and has the correct file extension. However, there are unnecessary f-strings without placeholders.
Replace the following lines with regular strings:
- self.logger.log(f'Found FLASHIda executable!') + self.logger.log('Found FLASHIda executable!')🧰 Tools
🪛 Ruff (0.8.2)
452-452: f-string without any placeholders
Remove extraneous
fprefix(F541)
457-470: Good validation of secondary executable.The code properly validates that the secondary FLASHIda executable exists and has the correct file extension. Same issue with f-strings without placeholders.
Replace the following lines with regular strings:
- self.logger.log(f'Found Secondary FLASHIda executable!') + self.logger.log('Found Secondary FLASHIda executable!')🧰 Tools
🪛 Ruff (0.8.2)
466-466: f-string without any placeholders
Remove extraneous
fprefix(F541)
471-491: Good validation of method and raw folders.The code properly validates that both the method files folder and raw files folder exist. Same issue with f-strings without placeholders.
Replace the following lines with regular strings:
- self.logger.log(f'Found methods folder!') - self.logger.log(f'Found raw file folder!') + self.logger.log('Found methods folder!') + self.logger.log('Found raw file folder!')🧰 Tools
🪛 Ruff (0.8.2)
474-474: f-string without any placeholders
Remove extraneous
fprefix(F541)
485-485: f-string without any placeholders
Remove extraneous
fprefix(F541)
516-522: Loop logic for file detection could be improved.The current for-else structure to find new raw files is not immediately obvious, and the static analysis tool flags unused variables. Consider restructuring this for clarity.
- # Search for new raws - new_raws, new_methods, new_secs = self._find_raws(raw_folder_path) - for raw, method, secondary in zip(new_raws, new_methods, new_secs): - if raw not in ign_raws: - break - else: - continue + # Search for new raws + new_raws, new_methods, new_secs = self._find_raws(raw_folder_path) + # Find first new raw file that hasn't been processed yet + new_file_found = False + for i, raw in enumerate(new_raws): + if raw not in ign_raws: + method = new_methods[i] + secondary = new_secs[i] + new_file_found = True + break + + if not new_file_found: + continue🧰 Tools
🪛 Ruff (0.8.2)
517-517: Loop control variable
methodnot used within loop body(B007)
517-517: Loop control variable
secondarynot used within loop body(B007)
533-535: Remove debugging log statements.The following lines appear to be debugging statements that should be removed before merging.
- self.logger.log(secondary) - self.logger.log(flashida_secondary_path)
538-538: Unnecessary f-string.Remove the f-prefix from strings without placeholders.
- self.logger.log(f'Starting FLASHDeconv...') + self.logger.log('Starting FLASHDeconv...')🧰 Tools
🪛 Ruff (0.8.2)
538-538: f-string without any placeholders
Remove extraneous
fprefix(F541)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Workflow.py(2 hunks)src/workflow/CommandExecutor.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/workflow/CommandExecutor.py (1)
src/workflow/Logger.py (1)
log(16-42)
🪛 Ruff (0.8.2)
src/workflow/CommandExecutor.py
80-80: f-string without any placeholders
Remove extraneous f prefix
(F541)
src/Workflow.py
452-452: f-string without any placeholders
Remove extraneous f prefix
(F541)
466-466: f-string without any placeholders
Remove extraneous f prefix
(F541)
474-474: f-string without any placeholders
Remove extraneous f prefix
(F541)
485-485: f-string without any placeholders
Remove extraneous f prefix
(F541)
517-517: Loop control variable method not used within loop body
(B007)
517-517: Loop control variable secondary not used within loop body
(B007)
538-538: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (5)
src/workflow/CommandExecutor.py (3)
63-63: Function signature enhancement looks good.The addition of the optional
cwdparameter torun_commandmethod allows specifying the working directory for command execution, which is a nice enhancement to support the new FLASHIda workflow.
76-78: Good logging of working directory.The code properly logs the working directory when it's provided, which helps with troubleshooting.
84-84: Appropriate implementation of working directory support.The
cwdparameter is correctly passed tosubprocess.Popen(), which is the standard way to specify the working directory for process execution in Python.src/Workflow.py (2)
414-421: Class initialization looks good.The
IdaWorkflowclass properly inherits fromWorkflowManagerand initializes with the correct workflow name "FLASHIda". The script path and tool name are appropriately set.
422-438: UI configuration is well-structured.The configuration method properly sets up four text input widgets for primary and secondary executable paths, and folder paths for raw and method files. This provides a clean user interface for the workflow configuration.
| def _find_raws(self, raw_path): | ||
| # Find existing raw files | ||
| raws = [] | ||
| methods = [] | ||
| secondary_flags = [] | ||
| method_pattern = r'.*FLASHIda_([^.]+)\.raw' | ||
| for file in listdir(raw_path): | ||
| if not Path(raw_path, file).is_file(): | ||
| continue | ||
| match = re.search(method_pattern, file) | ||
| if match: | ||
| raws.append(str(file)) | ||
| full_suffix = match.group(1) | ||
| parts = full_suffix.split('_', 1) | ||
| method = parts[0] | ||
| methods.append(method) | ||
| suffix = parts[1] if len(parts) > 1 else None | ||
| if suffix is not None: | ||
| suffix = suffix.split('_')[0] if '_' in suffix else suffix | ||
| suffix = suffix.split('.')[0] if '.' in suffix else suffix | ||
| secondary = suffix == '2' if suffix else False | ||
| secondary_flags.append(secondary) | ||
| return raws, methods, secondary_flags |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add docstring to _find_raws method.
The _find_raws method does complex regex pattern matching and string parsing but lacks a docstring explaining the expected file naming convention.
Add a descriptive docstring:
def _find_raws(self, raw_path):
+ """
+ Scans a directory for raw files matching the FLASHIda pattern.
+
+ Expected filename format: *FLASHIda_METHOD[_SUFFIX].raw
+ where METHOD is the name of the method file (without .xml extension)
+ and optional SUFFIX of '2' indicates using secondary executable.
+
+ Args:
+ raw_path (Path): Directory path to scan for raw files
+
+ Returns:
+ tuple: (list of raw filenames, list of method names, list of secondary flags)
+ """
# Find existing raw files📝 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.
| def _find_raws(self, raw_path): | |
| # Find existing raw files | |
| raws = [] | |
| methods = [] | |
| secondary_flags = [] | |
| method_pattern = r'.*FLASHIda_([^.]+)\.raw' | |
| for file in listdir(raw_path): | |
| if not Path(raw_path, file).is_file(): | |
| continue | |
| match = re.search(method_pattern, file) | |
| if match: | |
| raws.append(str(file)) | |
| full_suffix = match.group(1) | |
| parts = full_suffix.split('_', 1) | |
| method = parts[0] | |
| methods.append(method) | |
| suffix = parts[1] if len(parts) > 1 else None | |
| if suffix is not None: | |
| suffix = suffix.split('_')[0] if '_' in suffix else suffix | |
| suffix = suffix.split('.')[0] if '.' in suffix else suffix | |
| secondary = suffix == '2' if suffix else False | |
| secondary_flags.append(secondary) | |
| return raws, methods, secondary_flags | |
| def _find_raws(self, raw_path): | |
| """ | |
| Scans a directory for raw files matching the FLASHIda pattern. | |
| Expected filename format: *FLASHIda_METHOD[_SUFFIX].raw | |
| where METHOD is the name of the method file (without .xml extension) | |
| and optional SUFFIX of '2' indicates using secondary executable. | |
| Args: | |
| raw_path (Path): Directory path to scan for raw files | |
| Returns: | |
| tuple: (list of raw filenames, list of method names, list of secondary flags) | |
| """ | |
| # Find existing raw files | |
| raws = [] | |
| methods = [] | |
| secondary_flags = [] | |
| method_pattern = r'.*FLASHIda_([^.]+)\.raw' | |
| for file in listdir(raw_path): | |
| if not Path(raw_path, file).is_file(): | |
| continue | |
| match = re.search(method_pattern, file) | |
| if match: | |
| raws.append(str(file)) | |
| full_suffix = match.group(1) | |
| parts = full_suffix.split('_', 1) | |
| method = parts[0] | |
| methods.append(method) | |
| suffix = parts[1] if len(parts) > 1 else None | |
| if suffix is not None: | |
| suffix = suffix.split('_')[0] if '_' in suffix else suffix | |
| suffix = suffix.split('.')[0] if '.' in suffix else suffix | |
| secondary = suffix == '2' if suffix else False | |
| secondary_flags.append(secondary) | |
| return raws, methods, secondary_flags |
| while(True): | ||
| # Scan every 1s | ||
| sleep(1) | ||
|
|
||
| # Search for new raws |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding graceful termination for the infinite loop.
The workflow runs in an infinite loop with a 1-second sleep interval, which could cause issues if not properly terminated when the Streamlit app is closed.
Although the CommandExecutor has a stop method that kills processes, you should verify this works with infinite loops in the workflow. Consider adding a mechanism to break out of the loop when needed.
🏁 Script executed:
#!/bin/bash
# Check if WorkflowManager has a mechanism to stop infinite loops
rg -A 5 "def stop" "src/workflow/WorkflowManager.py"Length of output: 52
🏁 Script executed:
#!/bin/bash
# Locate the WorkflowManager class to verify its path
rg -n "class WorkflowManager" -n .
# Search for any stop/terminate methods across all Python files
rg -n "def stop" -n . | grep "\.py"
rg -n "def terminate" -n . | grep "\.py"Length of output: 243
Add graceful shutdown to the Workflow loop
The processing loop in src/Workflow.py (around lines 511–515) runs indefinitely with while True: sleep(1) and doesn’t observe any exit signal. Although CommandExecutor defines a stop() method, the loop never checks for it (or any flag), so closing the Streamlit app won’t break out of it.
To address this:
- Introduce a termination flag or
threading.Event(e.g.self._running = True). - Change
while(True):towhile self._running:. - Add a
stop()(orshutdown()) method to the Workflow class that setsself._running = False(or callsevent.set()). - Ensure this
stop()is invoked when the Streamlit app exits (usingst.on_exitor a teardown hook).
Locations needing updates:
- File:
src/Workflow.py(lines ~507–520): replace infinite loop and wire in the flag. - (Optional) The class that manages this workflow—add and expose the
stop()method for external callers.
This will let the workflow exit cleanly and prevent orphaned background polling when the app is closed.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
app.py (1)
3-3: Unused import is intentional for Windows compatibility.This issue was already addressed in a previous review. The pyopenms import is required for Windows compatibility despite not being explicitly used in the code.
src/Workflow.py (1)
514-552: Critical: Add graceful shutdown mechanism to infinite loop.The infinite loop issue was raised in previous reviews. The workflow runs indefinitely with
while(True): sleep(1)without any termination mechanism.This creates a potential issue where the process cannot be cleanly terminated when the Streamlit app is closed. Consider implementing a shutdown flag:
+ self._running = True self.logger.log('Listening for new raw files...') - while(True): + while self._running: # Scan every 1s sleep(1)Add a
stop()method to the workflow class:def stop(self): """Stop the workflow loop gracefully.""" self._running = False
🧹 Nitpick comments (8)
src/workflow/StreamlitUI.py (1)
863-865: Improve list parameter handling.The list parameter handling is good but could be more explicit about the conversion.
if isinstance(value, list): - value = '\n'.join(map(str, value)) - entry['widget_type'] = 'textarea' + # Convert list to newline-separated string for textarea input + value = '\n'.join(str(item) for item in value) + entry['widget_type'] = 'textarea'src/Workflow.py (7)
417-424: Fix excessive blank lines and formatting.Multiple formatting issues need to be addressed for better code consistency.
- -class IdaWorkflow(WorkflowManager): - +class IdaWorkflow(WorkflowManager): def __init__(self) -> None: # Initialize the parent class with the workflow name. super().__init__("FLASHIda", st.session_state["workspace"]) self.script_path = join('src', 'FLASHIda', 'run.py') self.tool_name = 'FLASHIdaRunner' -
447-494: Remove unnecessary f-string prefixes.Several f-strings don't contain placeholders and should be regular strings.
if flashida_path.is_file(): - self.logger.log(f'Found FLASHIda executable!') + self.logger.log('Found FLASHIda executable!') else: self.logger.log(f'{flashida_path} is not a file.') return # Validate secondary FLASHIda executable input flashida_secondary_path = Path(params['executable-secondary']) if flashida_secondary_path.suffix.lower() != '.exe': self.logger.log( f'Secondary FLASHIda executable was provided with extension ' f'\'{flashida_secondary_path.suffix}\'. Expected \'.exe\'' ) return if flashida_secondary_path.is_file(): - self.logger.log(f'Found Secondary FLASHIda executable!') + self.logger.log('Found Secondary FLASHIda executable!') else: self.logger.log(f'{flashida_secondary_path} is not a file.') return # Validate method file input methods_folder_path = Path(params['method-files']) if methods_folder_path.is_dir() and (params['method-files'] != ''): - self.logger.log(f'Found methods folder!') + self.logger.log('Found methods folder!') else: self.logger.log( f'Method folder \'{methods_folder_path}\' ' f'is not a folder.' ) return # Validate raw file input raw_folder_path = Path(params['raw-files']) if raw_folder_path.is_dir() and (params['raw-files'] != ''): - self.logger.log(f'Found raw file folder!') + self.logger.log('Found raw file folder!')
514-514: Fix formatting: add space after while keyword.- while(True): + while True:
541-551: Remove unnecessary f-string and fix formatting.if method_path.is_file(): self.logger.log(f'Found method \'{method_path}\'!') - self.logger.log(f'Starting FLASHDeconv...') + self.logger.log('Starting FLASHDeconv...') self.executor.run_command( [exe_path, '-m', method_path, '-r', raw_path], - cwd = exe_path.parent + cwd=exe_path.parent ) self.logger.log('Listening for new raw files...') else: self.logger.log( f'Method \'{method_path}\' is not valid. Ignoring...' ) self.logger.log('Listening for new raw files...')
581-589: Fix formatting issues in IdaSimulatorWorkflow.Multiple formatting issues need correction.
- -class IdaSimulatorWorkflow(WorkflowManager): - +class IdaSimulatorWorkflow(WorkflowManager): def __init__(self) -> None: # Initialize the parent class with the workflow name. super().__init__("FLASHIdaSimulator", st.session_state["workspace"]) self.script_path = join('scripts', 'flashida', 'write_method.py') self.tool_name = 'FLASHIdaRunner' - def upload(self)-> None: + def upload(self) -> None:
615-622: Remove unused variable and fix formatting.The
dataset_idvariable is assigned but never used, and there are formatting issues with string operations.# Generate output folder current_base = splitext(basename(in_mzml))[0] current_time = time.strftime("%Y%m%d-%H%M%S") - dataset_id = '%s_%s'%(current_base, current_time) - folder_path = join(base_path, 'FLASHIdaOutput', '%s_%s'%(current_base, current_time)) + folder_path = join(base_path, 'FLASHIdaOutput', f'{current_base}_{current_time}') makedirs(folder_path) # Generate temp paths for output files input_txt = join(folder_path, 'simulation_data.txt') input_xml = join(folder_path, 'method.xml') output_tsv = join(folder_path, 'simulation_data.txt') - -
642-650: Fix formatting issues in method execution.# Write method.xml self.logger.log('Generating parameter file...') self.executor.run_python( - self.script_path, {'input_xml' : join(folder_path, 'method.xml')} + self.script_path, {'input_xml': join(folder_path, 'method.xml')} ) # Run simulator self.logger.log('Running FLASHIda simulator...') self.executor.run_command( [params['executable'], input_txt, output_tsv, input_xml], - cwd = Path(params['executable']).parent + cwd=Path(params['executable']).parent ) - -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/build-windows-executable-app.yaml(1 hunks)app.py(2 hunks)content/FLASHIda/FLASHIdaSimulatorWorkflow.py(1 hunks)scripts/flashida/write_method.py(1 hunks)src/Workflow.py(2 hunks)src/workflow/CommandExecutor.py(3 hunks)src/workflow/StreamlitUI.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/build-windows-executable-app.yaml
- content/FLASHIda/FLASHIdaSimulatorWorkflow.py
🧰 Additional context used
🪛 Flake8 (7.2.0)
scripts/flashida/write_method.py
[error] 3-3: 'pyopenms as oms' imported but unused
(F401)
[error] 52-52: missing whitespace after ','
(E231)
[error] 68-68: expected 2 blank lines, found 1
(E302)
[error] 75-75: expected 2 blank lines after class or function definition, found 1
(E305)
src/Workflow.py
[error] 417-417: too many blank lines (3)
(E303)
[error] 455-455: f-string is missing placeholders
(F541)
[error] 469-469: f-string is missing placeholders
(F541)
[error] 477-477: f-string is missing placeholders
(F541)
[error] 488-488: f-string is missing placeholders
(F541)
[error] 514-514: missing whitespace after keyword
(E275)
[error] 541-541: f-string is missing placeholders
(F541)
[error] 544-544: unexpected spaces around keyword / parameter equals
(E251)
[error] 544-544: unexpected spaces around keyword / parameter equals
(E251)
[error] 555-555: too many blank lines (2)
(E303)
[error] 581-581: too many blank lines (3)
(E303)
[error] 589-589: missing whitespace around operator
(E225)
[error] 615-615: local variable 'dataset_id' is assigned to but never used
(F841)
[error] 615-615: missing whitespace around modulo operator
(E228)
[error] 616-616: missing whitespace around modulo operator
(E228)
[error] 625-625: too many blank lines (2)
(E303)
[error] 642-642: whitespace before ':'
(E203)
[error] 649-649: unexpected spaces around keyword / parameter equals
(E251)
[error] 649-649: unexpected spaces around keyword / parameter equals
(E251)
app.py
[error] 3-3: 'pyopenms' imported but unused
(F401)
src/workflow/CommandExecutor.py
[error] 80-80: f-string is missing placeholders
(F541)
🪛 Ruff (0.11.9)
scripts/flashida/write_method.py
3-3: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
src/Workflow.py
455-455: f-string without any placeholders
Remove extraneous f prefix
(F541)
469-469: f-string without any placeholders
Remove extraneous f prefix
(F541)
477-477: f-string without any placeholders
Remove extraneous f prefix
(F541)
488-488: f-string without any placeholders
Remove extraneous f prefix
(F541)
520-520: Loop control variable method not used within loop body
(B007)
520-520: Loop control variable secondary not used within loop body
(B007)
541-541: f-string without any placeholders
Remove extraneous f prefix
(F541)
615-615: Local variable dataset_id is assigned to but never used
Remove assignment to unused variable dataset_id
(F841)
app.py
3-3: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
src/workflow/CommandExecutor.py
80-80: f-string without any placeholders
Remove extraneous f prefix
(F541)
src/workflow/StreamlitUI.py
859-859: Use entry.get("value", None) instead of an if block
Replace with entry.get("value", None)
(SIM401)
🪛 Pylint (3.3.7)
src/Workflow.py
[refactor] 443-443: Too many local variables (20/15)
(R0914)
[refactor] 443-443: Too many branches (18/12)
(R0912)
[refactor] 443-443: Too many statements (54/50)
(R0915)
[refactor] 603-603: Too many local variables (17/15)
(R0914)
src/workflow/CommandExecutor.py
[refactor] 84-84: Consider using 'with' for resource-allocating operations
(R1732)
[error] 266-266: Iterated dict 'defaults' is being modified inside for loop body, iterate through a copy of it instead.
(E4702)
🔇 Additional comments (4)
app.py (1)
37-44: Good implementation of conditional page loading.The conditional addition of FLASHIda pages based on the
enable_flashidasetting is well implemented and follows the established pattern.src/workflow/StreamlitUI.py (2)
844-866: Good implementation of subsection support.The subsection header support is well implemented, allowing for better organization of parameters in the UI. The logic correctly handles string entries as section headers and prefixes subsequent parameters appropriately.
1134-1140: Simplify tool_text handling logic.The explicit handling of different tool_text cases is good for clarity and prevents potential issues with empty lists.
The explicit handling ensures robust behavior when
tool_textis empty, which prevents potential string formatting issues.src/workflow/CommandExecutor.py (1)
63-63: Method signature enhancement is well designed.The addition of the optional
cwdparameter torun_commandprovides useful flexibility for specifying working directories.
| DEFAULTS = [ | ||
| {"key": "in", "value": [], "help": "Input files for Python Script.", "hide": True}, | ||
| {'key': 'TopN', 'name': 'TopN', 'value': 3}, | ||
| {'key': 'Duration', 'name': 'Duration', 'value': 67}, | ||
| 'MS1', | ||
| {'key': 'Analyzer', 'name': 'Analyzer', 'value': 'Orbitrap'}, | ||
| {'key': 'FirstMass', 'name': 'FirstMass', 'value': 400}, | ||
| {'key': 'LastMass', 'name': 'LastMass', 'value': 2000}, | ||
| {'key': 'OrbitrapResolution', 'name': 'OrbitrapResolution', 'value': 120000}, | ||
| {'key': 'AGCTarget', 'name': 'AGCTarget', 'value': 800000}, | ||
| {'key': 'MaxIT', 'name': 'MaxIT', 'value': 50}, | ||
| {'key': 'Microscans', 'name': 'Microscans', 'value': 1}, | ||
| {'key': 'DataType', 'name': 'DataType', 'value': 'Centroid'}, | ||
| {'key': 'RFLens', 'name': 'RFLens', 'value': 30}, | ||
| {'key': 'SourceCID', 'name': 'SourceCID', 'value': 0}, | ||
| 'MS2', | ||
| {'key': 'Analyzer', 'name': 'Analyzer', 'value': 'Orbitrap'}, | ||
| {'key': 'FirstMass', 'name': 'FirstMass', 'value': 200}, | ||
| {'key': 'OrbitrapResolution', 'name': 'OrbitrapResolution', 'value': 60000}, | ||
| {'key': 'AGCTarget', 'name': 'AGCTarget', 'value': 500000}, | ||
| {'key': 'MaxIT', 'name': 'MaxIT', 'value': 118}, | ||
| {'key': 'Microscans', 'name': 'Microscans', 'value': 1}, | ||
| {'key': 'DataType', 'name': 'DataType', 'value': 'Centroid'}, | ||
| {'key': 'Activation', 'name': 'Activation', 'value': 'HCD'}, | ||
| {'key': 'CollisionEnergy', 'name': 'CollisionEnergy', 'value': 29}, | ||
| 'IDA', | ||
| {'key': 'MaxMs2CountPerMs1', 'name': 'MaxMs2CountPerMs1','value': 4}, | ||
| {'key': 'QScoreThreshold', 'name': 'QScoreThreshold', 'value': 0.2}, | ||
| {'key': 'MinCharge', 'name': 'MinCharge', 'value': 4}, | ||
| {'key': 'MaxCharge', 'name': 'MaxCharge', 'value': 50}, | ||
| {'key': 'MinMass', 'name': 'MinMass', 'value': 500}, | ||
| {'key': 'MaxMass', 'name': 'MaxMass', 'value': 50000}, | ||
| {'key': 'Tolerances', 'name': 'Tolerances', 'value': [10.0, 10.0]}, | ||
| {'key': 'TargetLogs', 'name': 'TargetLogs', 'value': [r'C:\Users\KyowonJeong\Desktop\FLASHIdaTmp\test1.log']}, | ||
| {'key': 'RTWindow', 'name': 'RTWindow', 'value': 180}, | ||
| {'key': 'TargetMode', 'name': 'TargetMode', 'value': 0}, | ||
| {'key': 'UseFAIMS', 'name': 'UseFAIMS', 'value': False}, | ||
| {'key': 'UseCVQScore', 'name': 'UseCVQScore', 'value': False}, | ||
| {'key': 'CycleTime', 'name': 'CycleTime', 'value': 180}, | ||
| {'key': 'CVValues', 'name': 'CVValues', 'value': [-10.0, -30.0, -40.0, -50.0, -60.0]}, | ||
| ] |
There was a problem hiding this comment.
Fix DEFAULTS structure and formatting issues.
There are several issues with the DEFAULTS list:
- Line 30 contains a string literal
'MS1'that should be a dictionary entry - Missing whitespace after comma on line 52
- The structure mixes strings (section headers) and dictionaries inconsistently
{"key": "in", "value": [], "help": "Input files for Python Script.", "hide": True},
{'key': 'TopN', 'name': 'TopN', 'value': 3},
{'key': 'Duration', 'name': 'Duration', 'value': 67},
- 'MS1',
+ 'MS1',
{'key': 'Analyzer', 'name': 'Analyzer', 'value': 'Orbitrap'},
{'key': 'FirstMass', 'name': 'FirstMass', 'value': 400},
{'key': 'LastMass', 'name': 'LastMass', 'value': 2000},
{'key': 'OrbitrapResolution', 'name': 'OrbitrapResolution', 'value': 120000},
{'key': 'AGCTarget', 'name': 'AGCTarget', 'value': 800000},
{'key': 'MaxIT', 'name': 'MaxIT', 'value': 50},
{'key': 'Microscans', 'name': 'Microscans', 'value': 1},
{'key': 'DataType', 'name': 'DataType', 'value': 'Centroid'},
{'key': 'RFLens', 'name': 'RFLens', 'value': 30},
{'key': 'SourceCID', 'name': 'SourceCID', 'value': 0},
'MS2',
{'key': 'Analyzer', 'name': 'Analyzer', 'value': 'Orbitrap'},
{'key': 'FirstMass', 'name': 'FirstMass', 'value': 200},
{'key': 'OrbitrapResolution', 'name': 'OrbitrapResolution', 'value': 60000},
{'key': 'AGCTarget', 'name': 'AGCTarget', 'value': 500000},
{'key': 'MaxIT', 'name': 'MaxIT', 'value': 118},
{'key': 'Microscans', 'name': 'Microscans', 'value': 1},
{'key': 'DataType', 'name': 'DataType', 'value': 'Centroid'},
{'key': 'Activation', 'name': 'Activation', 'value': 'HCD'},
{'key': 'CollisionEnergy', 'name': 'CollisionEnergy', 'value': 29},
'IDA',
- {'key': 'MaxMs2CountPerMs1', 'name': 'MaxMs2CountPerMs1','value': 4},
+ {'key': 'MaxMs2CountPerMs1', 'name': 'MaxMs2CountPerMs1', 'value': 4},Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 52-52: missing whitespace after ','
(E231)
🤖 Prompt for AI Agents
In scripts/flashida/write_method.py between lines 26 and 66, the DEFAULTS list
improperly mixes string literals like 'MS1' as section headers with dictionary
entries, causing inconsistent structure. Replace these string literals with
dictionary entries that clearly mark sections, for example using a key like
'section' or similar. Also, fix the missing whitespace after the comma on line
52 to maintain proper formatting and readability. Ensure the entire DEFAULTS
list consistently uses dictionaries for all entries.
| if __name__ == "__main__": | ||
| params = get_params() | ||
|
|
||
| input_xml = params.pop('input_xml') | ||
|
|
||
| # Create method.xml | ||
| root = ET.Element("MethodParameters") | ||
| subsections = {} | ||
| for key, value in params.items(): | ||
| if ':' in key: | ||
| section, param = key.split(':') | ||
| if section not in subsections: | ||
| subsections[section] = ET.SubElement(root, section) | ||
| section = subsections[section] | ||
| tag = ET.SubElement(section, param) | ||
| else: | ||
| tag = ET.SubElement(root, key) | ||
| # Handle list inputs | ||
| if key in ['IDA:Tolerances', 'IDA:CVValues']: | ||
| for item in value.split('\n'): | ||
| element = ET.SubElement(tag, 'double') | ||
| element.text = item.strip() | ||
| elif key in ['IDA:TargetLogs']: | ||
| for item in value.split('\n'): | ||
| element = ET.SubElement(tag, 'string') | ||
| element.text = item.strip().replace('\\\\', '\\') | ||
| else: | ||
| tag.text = str(value) | ||
| tree = ET.ElementTree(root) | ||
| ET.indent(tree, space=" ") | ||
| tree.write(input_xml, encoding="utf-8", xml_declaration=True) No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper spacing and improve error handling.
The main execution block needs better spacing and error handling for missing keys.
+
if __name__ == "__main__":
params = get_params()
- input_xml = params.pop('input_xml')
+ if 'input_xml' not in params:
+ raise ValueError("Missing required 'input_xml' parameter")
+ input_xml = params.pop('input_xml')
# Create method.xml
root = ET.Element("MethodParameters")
subsections = {}
for key, value in params.items():
if ':' in key:
section, param = key.split(':')
if section not in subsections:
subsections[section] = ET.SubElement(root, section)
section = subsections[section]
tag = ET.SubElement(section, param)
else:
tag = ET.SubElement(root, key)
# Handle list inputs
if key in ['IDA:Tolerances', 'IDA:CVValues']:
for item in value.split('\n'):
element = ET.SubElement(tag, 'double')
element.text = item.strip()
elif key in ['IDA:TargetLogs']:
for item in value.split('\n'):
element = ET.SubElement(tag, 'string')
element.text = item.strip().replace('\\\\', '\\')
else:
tag.text = str(value)
tree = ET.ElementTree(root)
ET.indent(tree, space=" ")
tree.write(input_xml, encoding="utf-8", xml_declaration=True)📝 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.
| if __name__ == "__main__": | |
| params = get_params() | |
| input_xml = params.pop('input_xml') | |
| # Create method.xml | |
| root = ET.Element("MethodParameters") | |
| subsections = {} | |
| for key, value in params.items(): | |
| if ':' in key: | |
| section, param = key.split(':') | |
| if section not in subsections: | |
| subsections[section] = ET.SubElement(root, section) | |
| section = subsections[section] | |
| tag = ET.SubElement(section, param) | |
| else: | |
| tag = ET.SubElement(root, key) | |
| # Handle list inputs | |
| if key in ['IDA:Tolerances', 'IDA:CVValues']: | |
| for item in value.split('\n'): | |
| element = ET.SubElement(tag, 'double') | |
| element.text = item.strip() | |
| elif key in ['IDA:TargetLogs']: | |
| for item in value.split('\n'): | |
| element = ET.SubElement(tag, 'string') | |
| element.text = item.strip().replace('\\\\', '\\') | |
| else: | |
| tag.text = str(value) | |
| tree = ET.ElementTree(root) | |
| ET.indent(tree, space=" ") | |
| tree.write(input_xml, encoding="utf-8", xml_declaration=True) | |
| if __name__ == "__main__": | |
| params = get_params() | |
| if 'input_xml' not in params: | |
| raise ValueError("Missing required 'input_xml' parameter") | |
| input_xml = params.pop('input_xml') | |
| # Create method.xml | |
| root = ET.Element("MethodParameters") | |
| subsections = {} | |
| for key, value in params.items(): | |
| if ':' in key: | |
| section, param = key.split(':') | |
| if section not in subsections: | |
| subsections[section] = ET.SubElement(root, section) | |
| section = subsections[section] | |
| tag = ET.SubElement(section, param) | |
| else: | |
| tag = ET.SubElement(root, key) | |
| # Handle list inputs | |
| if key in ['IDA:Tolerances', 'IDA:CVValues']: | |
| for item in value.split('\n'): | |
| element = ET.SubElement(tag, 'double') | |
| element.text = item.strip() | |
| elif key in ['IDA:TargetLogs']: | |
| for item in value.split('\n'): | |
| element = ET.SubElement(tag, 'string') | |
| element.text = item.strip().replace('\\\\', '\\') | |
| else: | |
| tag.text = str(value) | |
| tree = ET.ElementTree(root) | |
| ET.indent(tree, space=" ") | |
| tree.write(input_xml, encoding="utf-8", xml_declaration=True) |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 75-75: expected 2 blank lines after class or function definition, found 1
(E305)
🤖 Prompt for AI Agents
In scripts/flashida/write_method.py around lines 75 to 105, improve the main
execution block by adding proper blank lines for readability and include error
handling for missing keys in the params dictionary, especially for 'input_xml'.
Wrap the key access in a try-except block to catch KeyError and provide a clear
error message or fallback behavior. Also, ensure consistent spacing between
logical code sections to enhance clarity.
| def get_params(): | ||
| if len(sys.argv) > 1: | ||
| with open(sys.argv[1], "r") as f: | ||
| return json.load(f) | ||
| else: | ||
| return {} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper function spacing and improve function design.
The function lacks proper spacing and could be more robust.
+
def get_params():
if len(sys.argv) > 1:
with open(sys.argv[1], "r") as f:
return json.load(f)
else:
return {}
+📝 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.
| def get_params(): | |
| if len(sys.argv) > 1: | |
| with open(sys.argv[1], "r") as f: | |
| return json.load(f) | |
| else: | |
| return {} | |
| def get_params(): | |
| if len(sys.argv) > 1: | |
| with open(sys.argv[1], "r") as f: | |
| return json.load(f) | |
| else: | |
| return {} | |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 68-68: expected 2 blank lines, found 1
(E302)
🤖 Prompt for AI Agents
In scripts/flashida/write_method.py around lines 68 to 74, the get_params
function lacks proper spacing and could be improved for robustness. Add a blank
line before the function definition for proper spacing. Enhance the function by
adding error handling for file operations and JSON loading to prevent crashes if
the file is missing or contains invalid JSON. Also, consider validating the
input argument count more explicitly.
| # Log cwd | ||
| if cwd is not None: | ||
| self.logger.log(f'Preparing command... cwd is set to \'{cwd}\'') | ||
| # Log the execution start | ||
| self.logger.log(f"Running command:\n"+' '.join(command)+"\nWaiting for command to finish...", 1) | ||
| start_time = time.time() | ||
|
|
||
| # Execute the command | ||
| process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
| process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) |
There was a problem hiding this comment.
Fix f-string and improve resource management.
There are two issues here:
- Line 80 has an f-string without placeholders
subprocess.Popenshould use context manager for proper resource management
# Log cwd
if cwd is not None:
- self.logger.log(f'Preparing command... cwd is set to \'{cwd}\'')
+ self.logger.log(f'Preparing command... cwd is set to \'{cwd}\'')
# Log the execution start
- self.logger.log(f"Running command:\n"+' '.join(command)+"\nWaiting for command to finish...", 1)
+ self.logger.log("Running command:\n" + ' '.join(command) + "\nWaiting for command to finish...", 1)
start_time = time.time()
# Execute the command
- process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd)
+ with subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) as process:Note: You'll also need to adjust the indentation of the subsequent code that uses the process variable.
📝 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.
| # Log cwd | |
| if cwd is not None: | |
| self.logger.log(f'Preparing command... cwd is set to \'{cwd}\'') | |
| # Log the execution start | |
| self.logger.log(f"Running command:\n"+' '.join(command)+"\nWaiting for command to finish...", 1) | |
| start_time = time.time() | |
| # Execute the command | |
| process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | |
| process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) | |
| # Log cwd | |
| if cwd is not None: | |
| self.logger.log(f'Preparing command... cwd is set to \'{cwd}\'') | |
| # Log the execution start | |
| self.logger.log("Running command:\n" + ' '.join(command) + "\nWaiting for command to finish...", 1) | |
| start_time = time.time() | |
| # Execute the command | |
| with subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) as process: | |
| # … (subsequent code that uses `process` needs to be indented here) |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 80-80: f-string is missing placeholders
(F541)
🪛 Ruff (0.11.9)
80-80: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Pylint (3.3.7)
[refactor] 84-84: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In src/workflow/CommandExecutor.py around lines 76 to 84, fix the f-string on
line 80 by removing the unnecessary f prefix since there are no placeholders,
and refactor the subprocess.Popen call to use a context manager (with statement)
to ensure proper resource management. Also, adjust the indentation of all
subsequent code that interacts with the process variable to be inside the with
block.
| subsection = '' | ||
| defaults = {} | ||
| for entry in defaults: | ||
| if isinstance(entry, str): | ||
| subsection = entry | ||
| continue | ||
| defaults[f'{subsection}:{entry["key"]}'] = entry["value"] | ||
| # load paramters from JSON file |
There was a problem hiding this comment.
Fix dictionary iteration issue.
Modifying a dictionary while iterating over it can cause issues. The current approach of building a new dictionary is safer.
The current implementation correctly avoids the iteration issue by building a new defaults dictionary rather than modifying the original list during iteration. However, the variable name reuse could be confusing.
elif isinstance(defaults, list):
subsection = ''
- defaults = {}
+ defaults_dict = {}
for entry in defaults:
if isinstance(entry, str):
subsection = entry
continue
- defaults[f'{subsection}:{entry["key"]}'] = entry["value"]
+ defaults_dict[f'{subsection}:{entry["key"]}'] = entry["value"]
# load paramters from JSON file
params = {k: v for k, v in self.parameter_manager.get_parameters_from_json().items() if path.name in k}
# update defaults
for k, v in params.items():
- defaults[k.replace(f"{path.name}:", "")] = v
+ defaults_dict[k.replace(f"{path.name}:", "")] = v
for k, v in input_output.items():
- defaults[k] = v
+ defaults_dict[k] = v
# save parameters to temporary JSON file
tmp_params_file = Path(self.pid_dir.parent, f"{path.stem}.json")
with open(tmp_params_file, "w", encoding="utf-8") as f:
- json.dump(defaults, f, indent=4)
+ json.dump(defaults_dict, f, 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.
| subsection = '' | |
| defaults = {} | |
| for entry in defaults: | |
| if isinstance(entry, str): | |
| subsection = entry | |
| continue | |
| defaults[f'{subsection}:{entry["key"]}'] = entry["value"] | |
| # load paramters from JSON file | |
| elif isinstance(defaults, list): | |
| subsection = '' | |
| defaults_dict = {} | |
| for entry in defaults: | |
| if isinstance(entry, str): | |
| subsection = entry | |
| continue | |
| defaults_dict[f'{subsection}:{entry["key"]}'] = entry["value"] | |
| # load parameters from JSON file | |
| params = { | |
| k: v | |
| for k, v in self.parameter_manager.get_parameters_from_json().items() | |
| if path.name in k | |
| } | |
| # update defaults | |
| for k, v in params.items(): | |
| defaults_dict[k.replace(f"{path.name}:", "")] = v | |
| for k, v in input_output.items(): | |
| defaults_dict[k] = v | |
| # save parameters to temporary JSON file | |
| tmp_params_file = Path(self.pid_dir.parent, f"{path.stem}.json") | |
| with open(tmp_params_file, "w", encoding="utf-8") as f: | |
| json.dump(defaults_dict, f, indent=4) |
🧰 Tools
🪛 Pylint (3.3.7)
[error] 266-266: Iterated dict 'defaults' is being modified inside for loop body, iterate through a copy of it instead.
(E4702)
🤖 Prompt for AI Agents
In src/workflow/CommandExecutor.py around lines 260 to 267, the variable
'defaults' is used both as the original iterable and as the new dictionary being
built, which can be confusing. Rename the original iterable variable to a
different name (e.g., 'default_entries') to avoid reusing 'defaults' and clarify
the code, while keeping the logic that builds a new dictionary instead of
modifying the iterable during iteration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Workflow.py (2)
555-577: Fix formatting and address previous review feedback.
- Remove excessive blank lines:
- def _find_raws(self, raw_path): + def _find_raws(self, raw_path):The previous review comment about adding a docstring to this method is still valid and should be addressed.
443-553: Multiple issues need addressing in this complex method.This method has several problems that should be fixed:
- F-strings without placeholders (lines 455, 469, 477, 488, 541):
- self.logger.log(f'Found FLASHIda executable!') + self.logger.log('Found FLASHIda executable!')
- Formatting issues (line 514):
- while(True): + while True:
- Unused loop variables (line 520):
- for raw, method, secondary in zip(new_raws, new_methods, new_secs): + for raw, _, _ in zip(new_raws, new_methods, new_secs):
- Parameter spacing (line 544):
- cwd = exe_path.parent + cwd=exe_path.parentThe infinite loop issue was already flagged in previous reviews - this method needs graceful shutdown capability to prevent orphaned processes when the Streamlit app closes.
🧹 Nitpick comments (3)
src/Workflow.py (3)
417-424: Fix formatting: Remove excessive blank lines.There are too many blank lines before the class definition.
- - class IdaWorkflow(WorkflowManager): +class IdaWorkflow(WorkflowManager):
425-442: Consider adding help text for better user guidance.The configuration inputs lack help text that would guide users on expected file formats and paths.
Consider adding help parameters to provide user guidance:
self.ui.input_widget( key="executable", name="Path to Flash.exe", default='', - widget_type="text" + widget_type="text", + help="Full path to the primary FLASHIda executable (.exe file)" )
581-588: Fix formatting issues.Multiple formatting problems need to be addressed:
- Too many blank lines (line 581):
- class IdaSimulatorWorkflow(WorkflowManager): +class IdaSimulatorWorkflow(WorkflowManager):
- Missing whitespace around operator (line 589):
- def upload(self)-> None: + def upload(self) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-windows-executable-app.yaml(1 hunks)app.py(1 hunks)settings.json(1 hunks)src/Workflow.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- settings.json
- .github/workflows/build-windows-executable-app.yaml
- app.py
🧰 Additional context used
🪛 Ruff (0.11.9)
src/Workflow.py
455-455: f-string without any placeholders
Remove extraneous f prefix
(F541)
469-469: f-string without any placeholders
Remove extraneous f prefix
(F541)
477-477: f-string without any placeholders
Remove extraneous f prefix
(F541)
488-488: f-string without any placeholders
Remove extraneous f prefix
(F541)
520-520: Loop control variable method not used within loop body
(B007)
520-520: Loop control variable secondary not used within loop body
(B007)
541-541: f-string without any placeholders
Remove extraneous f prefix
(F541)
615-615: Local variable dataset_id is assigned to but never used
Remove assignment to unused variable dataset_id
(F841)
🪛 Flake8 (7.2.0)
src/Workflow.py
[error] 417-417: too many blank lines (3)
(E303)
[error] 455-455: f-string is missing placeholders
(F541)
[error] 469-469: f-string is missing placeholders
(F541)
[error] 477-477: f-string is missing placeholders
(F541)
[error] 488-488: f-string is missing placeholders
(F541)
[error] 514-514: missing whitespace after keyword
(E275)
[error] 541-541: f-string is missing placeholders
(F541)
[error] 544-544: unexpected spaces around keyword / parameter equals
(E251)
[error] 544-544: unexpected spaces around keyword / parameter equals
(E251)
[error] 555-555: too many blank lines (2)
(E303)
[error] 581-581: too many blank lines (3)
(E303)
[error] 589-589: missing whitespace around operator
(E225)
[error] 615-615: local variable 'dataset_id' is assigned to but never used
(F841)
[error] 615-615: missing whitespace around modulo operator
(E228)
[error] 616-616: missing whitespace around modulo operator
(E228)
[error] 625-625: too many blank lines (2)
(E303)
[error] 642-642: whitespace before ':'
(E203)
[error] 649-649: unexpected spaces around keyword / parameter equals
(E251)
[error] 649-649: unexpected spaces around keyword / parameter equals
(E251)
🪛 Pylint (3.3.7)
src/Workflow.py
[refactor] 443-443: Too many local variables (20/15)
(R0914)
[refactor] 443-443: Too many branches (18/12)
(R0912)
[refactor] 443-443: Too many statements (54/50)
(R0915)
[refactor] 603-603: Too many local variables (17/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (2)
src/Workflow.py (2)
1-1: LGTM - New imports support FLASHIda functionality.The new imports (
re,pyopenms,sleep) are appropriately added to support the regex pattern matching, mass spectrometry data processing, and timing functionality needed by the new FLASHIda workflows.Also applies to: 7-7, 9-9
592-602: LGTM - Clean and appropriate configuration.The configuration method appropriately sets up the necessary inputs for the simulator workflow with file selection, executable path, and Python script configuration.
| def execution(self) -> None: | ||
| params = self.parameter_manager.get_parameters_from_json() | ||
|
|
||
| # Make sure output directory exists | ||
| base_path = dirname(self.workflow_dir) | ||
|
|
||
| # Get input files | ||
| in_mzml = self.file_manager.get_files(self.params["mzML-files"])[0] | ||
|
|
||
| # Generate output folder | ||
| current_base = splitext(basename(in_mzml))[0] | ||
| current_time = time.strftime("%Y%m%d-%H%M%S") | ||
| dataset_id = '%s_%s'%(current_base, current_time) | ||
| folder_path = join(base_path, 'FLASHIdaOutput', '%s_%s'%(current_base, current_time)) | ||
| makedirs(folder_path) | ||
|
|
||
| # Generate temp paths for output files | ||
| input_txt = join(folder_path, 'simulation_data.txt') | ||
| input_xml = join(folder_path, 'method.xml') | ||
| output_tsv = join(folder_path, 'simulation_data.txt') | ||
|
|
||
|
|
||
| # Convert input mzML to input format | ||
| self.logger.log('Converting mzML to simulation input format...') | ||
| exp = oms.MSExperiment() | ||
| oms.MzMLFile().load(in_mzml, exp) | ||
| output = "" | ||
| for s in exp.getSpectra(): | ||
| if s.getMSLevel() > 1: | ||
| continue | ||
| output += f'Spec\t{s.getRT()}\n' | ||
| for mz, intensity in zip(*s.get_peaks()): | ||
| output += f'{mz}\t{intensity}\n' | ||
| with open(input_txt, 'w') as f: | ||
| f.write(output) | ||
|
|
||
| # Write method.xml | ||
| self.logger.log('Generating parameter file...') | ||
| self.executor.run_python( | ||
| self.script_path, {'input_xml' : join(folder_path, 'method.xml')} | ||
| ) | ||
|
|
||
| # Run simulator | ||
| self.logger.log('Running FLASHIda simulator...') | ||
| self.executor.run_command( | ||
| [params['executable'], input_txt, output_tsv, input_xml], | ||
| cwd = Path(params['executable']).parent | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix multiple issues in the execution method.
Several problems need to be addressed:
- Unused variable (line 615):
- dataset_id = '%s_%s'%(current_base, current_time)
+ # dataset_id = '%s_%s'%(current_base, current_time) # Unused, keeping for potential future use- Formatting issues (lines 615-616, 625, 642, 649):
- dataset_id = '%s_%s'%(current_base, current_time)
- folder_path = join(base_path, 'FLASHIdaOutput', '%s_%s'%(current_base, current_time))
+ dataset_id = '%s_%s' % (current_base, current_time)
+ folder_path = join(base_path, 'FLASHIdaOutput', '%s_%s' % (current_base, current_time))
- self.executor.run_python(
- self.script_path, {'input_xml' : join(folder_path, 'method.xml')}
- )
+ self.executor.run_python(
+ self.script_path, {'input_xml': join(folder_path, 'method.xml')}
+ )
- cwd = Path(params['executable']).parent
+ cwd=Path(params['executable']).parent- Remove excessive blank lines (line 625):
-
# Convert input mzML to input format
+ # Convert input mzML to input formatNote: This method has high complexity with 17 local variables. Consider refactoring into smaller, more focused methods for better maintainability.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.11.9)
615-615: Local variable dataset_id is assigned to but never used
Remove assignment to unused variable dataset_id
(F841)
🪛 Flake8 (7.2.0)
[error] 615-615: local variable 'dataset_id' is assigned to but never used
(F841)
[error] 615-615: missing whitespace around modulo operator
(E228)
[error] 616-616: missing whitespace around modulo operator
(E228)
[error] 625-625: too many blank lines (2)
(E303)
[error] 642-642: whitespace before ':'
(E203)
[error] 649-649: unexpected spaces around keyword / parameter equals
(E251)
[error] 649-649: unexpected spaces around keyword / parameter equals
(E251)
🪛 Pylint (3.3.7)
[refactor] 603-603: Too many local variables (17/15)
(R0914)
🤖 Prompt for AI Agents
In src/Workflow.py between lines 603 and 651, fix the execution method by
removing the unused variable 'output' declared at line 615, correct formatting
issues such as inconsistent spacing and indentation around lines 615-616, 625,
642, and 649, and eliminate excessive blank lines especially around line 625.
Additionally, consider refactoring this method into smaller helper methods to
reduce complexity and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
scripts/flashida/write_method.py (2)
51-51: Fix missing whitespace after comma.The missing whitespace after comma on line 51 was already identified in previous reviews and needs to be fixed.
67-73: Add proper function spacing.The function spacing issues around lines 67-74 were already identified in previous reviews and need to be addressed.
src/Workflow.py (1)
515-520: Consider adding graceful termination for the infinite loop.This infinite loop concern was already raised in previous reviews and should be addressed to allow proper cleanup when the Streamlit app is closed.
🧹 Nitpick comments (13)
src/parse/ida.py (2)
24-27: Remove unnecessary f-string prefix.The f-string on line 26 has no placeholders and should be a regular string.
file_manager.store_data( - dataset_id, f'ms1_deconv_heatmap', heatmap + dataset_id, 'ms1_deconv_heatmap', heatmap )
30-34: Fix formatting and remove excessive blank lines.Remove extra blank lines and ensure consistent formatting.
for size in reversed(compute_compression_levels(20000, len(heatmap), logger=logger)): - - # Downsample iteratively heatmap = downsample_heatmap(heatmap, max_datapoints=size)content/FLASHIda/FLASHIdaViewer.py (4)
11-18: Add proper spacing before functions.Missing blank lines before function definitions reduce readability.
DEFAULT_LAYOUT = [['ms1_deconv_heat_map']] + def select_experiment():
19-27: Add proper spacing and improve error handling.Missing blank lines and could benefit from better structure.
+ def validate_selected_index(file_manager, selected_experiment): results = file_manager.get_results_list(['deconv_dfs', 'anno_dfs']) if selected_experiment in st.session_state: if st.session_state[selected_experiment] in results: return name_to_index[st.session_state[selected_experiment]] - else: - del st.session_state[selected_experiment] + del st.session_state[selected_experiment] return None +
73-77: Remove unnecessary f-string prefixes.Lines 73 and 77 have f-strings without placeholders.
st.selectbox( "choose experiment", results, - key=f'selected_experiment_dropdown_1', + key='selected_experiment_dropdown_1', index=validate_selected_index(file_manager, 'selected_experiment1'), on_change=select_experiment ) - if f"selected_experiment1" in st.session_state: + if "selected_experiment1" in st.session_state:
104-105: Fix multiple statements on one line.Separate the continue statement onto its own line for better readability.
for exp_index, exp_layout in enumerate(layout): - if exp_index == 0: continue # skip the first experiment + if exp_index == 0: + continue # skip the first experimentsrc/Workflow.py (7)
417-418: Remove excessive blank lines.Too many blank lines before the class definition reduce readability.
- + class IdaWorkflow(WorkflowManager):
456-456: Remove unnecessary f-string prefix.Multiple f-strings throughout the code don't have placeholders and should be regular strings.
- self.logger.log(f'Found FLASHIda executable!') + self.logger.log('Found FLASHIda executable!')
515-515: Fix spacing around keyword.Missing space after the
whilekeyword.- while(True): + while True:
521-525: Fix unused loop variables.The loop variables
methodandsecondaryare not used within the loop body.- for raw, method, secondary in zip(new_raws, new_methods, new_secs): + for raw, _, _ in zip(new_raws, new_methods, new_secs): if raw not in ign_raws: break
616-617: Fix spacing around modulo operators.Missing spaces around the modulo operators reduce readability.
- dataset_id = '%s_%s'%(current_base, current_time) - folder_path = join(base_path, 'FLASHIdaOutput', '%s_%s'%(current_base, current_time)) + dataset_id = '%s_%s' % (current_base, current_time) + folder_path = join(base_path, 'FLASHIdaOutput', '%s_%s' % (current_base, current_time))
642-642: Fix spacing around colon.Remove extra whitespace before the colon.
- self.script_path, {'input_xml' : join(folder_path, 'method.xml')} + self.script_path, {'input_xml': join(folder_path, 'method.xml')}
649-649: Fix spacing around keyword argument.Remove extra spaces around the equals sign in keyword arguments.
- cwd = Path(params['executable']).parent + cwd=Path(params['executable']).parent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app.py(1 hunks)content/FLASHIda/FLASHIdaViewer.py(1 hunks)scripts/flashida/write_method.py(1 hunks)src/Workflow.py(2 hunks)src/parse/ida.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/parse/ida.py (3)
src/parse/masstable.py (2)
getMSSignalDF(195-240)getSpectraTableDF(188-192)src/render/compression.py (2)
downsample_heatmap(18-56)compute_compression_levels(7-15)src/workflow/FileManager.py (1)
store_data(309-327)
🪛 Flake8 (7.2.0)
content/FLASHIda/FLASHIdaViewer.py
[error] 11-11: expected 2 blank lines, found 1
(E302)
[error] 19-19: expected 2 blank lines, found 1
(E302)
[error] 29-29: expected 2 blank lines after class or function definition, found 1
(E305)
[error] 48-48: too many leading '#' for block comment
(E266)
[error] 54-54: whitespace before ':'
(E203)
[error] 73-73: f-string is missing placeholders
(F541)
[error] 77-77: f-string is missing placeholders
(F541)
[error] 86-86: too many leading '#' for block comment
(E266)
[error] 95-95: too many blank lines (2)
(E303)
[error] 101-101: too many leading '#' for block comment
(E266)
[error] 105-105: multiple statements on one line (colon)
(E701)
src/parse/ida.py
[error] 2-2: 'numpy as np' imported but unused
(F401)
[error] 4-4: 'src.parse.masstable.parseFLASHDeconvOutput' imported but unused
(F401)
[error] 4-4: 'src.parse.masstable.getMSSignalDF' imported but unused
(F401)
[error] 4-4: 'src.parse.masstable.getSpectraTableDF' imported but unused
(F401)
[error] 6-6: 'scipy.stats.gaussian_kde' imported but unused
(F401)
[error] 8-8: expected 2 blank lines, found 1
(E302)
[error] 17-17: missing whitespace after ','
(E231)
[error] 20-20: whitespace before ':'
(E203)
[error] 20-20: whitespace before ':'
(E203)
[error] 21-21: whitespace before ':'
(E203)
[error] 26-26: f-string is missing placeholders
(F541)
[error] 33-33: too many blank lines (2)
(E303)
scripts/flashida/write_method.py
[error] 51-51: missing whitespace after ','
(E231)
[error] 67-67: expected 2 blank lines, found 1
(E302)
[error] 74-74: expected 2 blank lines after class or function definition, found 1
(E305)
src/Workflow.py
[error] 418-418: too many blank lines (3)
(E303)
[error] 456-456: f-string is missing placeholders
(F541)
[error] 470-470: f-string is missing placeholders
(F541)
[error] 478-478: f-string is missing placeholders
(F541)
[error] 489-489: f-string is missing placeholders
(F541)
[error] 515-515: missing whitespace after keyword
(E275)
[error] 542-542: f-string is missing placeholders
(F541)
[error] 545-545: unexpected spaces around keyword / parameter equals
(E251)
[error] 545-545: unexpected spaces around keyword / parameter equals
(E251)
[error] 556-556: too many blank lines (2)
(E303)
[error] 582-582: too many blank lines (3)
(E303)
[error] 590-590: missing whitespace around operator
(E225)
[error] 616-616: missing whitespace around modulo operator
(E228)
[error] 617-617: missing whitespace around modulo operator
(E228)
[error] 626-626: too many blank lines (2)
(E303)
[error] 642-642: whitespace before ':'
(E203)
[error] 649-649: unexpected spaces around keyword / parameter equals
(E251)
[error] 649-649: unexpected spaces around keyword / parameter equals
(E251)
🪛 Pylint (3.3.7)
content/FLASHIda/FLASHIdaViewer.py
[refactor] 22-25: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
src/Workflow.py
[refactor] 444-444: Too many local variables (20/15)
(R0914)
[refactor] 444-444: Too many branches (18/12)
(R0912)
[refactor] 444-444: Too many statements (54/50)
(R0915)
[refactor] 604-604: Too many local variables (17/15)
(R0914)
🪛 Ruff (0.11.9)
content/FLASHIda/FLASHIdaViewer.py
73-73: f-string without any placeholders
Remove extraneous f prefix
(F541)
77-77: f-string without any placeholders
Remove extraneous f prefix
(F541)
104-104: Loop control variable exp_layout not used within loop body
Rename unused exp_layout to _exp_layout
(B007)
105-105: Multiple statements on one line (colon)
(E701)
src/parse/ida.py
2-2: numpy imported but unused
Remove unused import: numpy
(F401)
4-4: src.parse.masstable.parseFLASHDeconvOutput imported but unused
Remove unused import
(F401)
4-4: src.parse.masstable.getMSSignalDF imported but unused
Remove unused import
(F401)
4-4: src.parse.masstable.getSpectraTableDF imported but unused
Remove unused import
(F401)
6-6: scipy.stats.gaussian_kde imported but unused
Remove unused import: scipy.stats.gaussian_kde
(F401)
26-26: f-string without any placeholders
Remove extraneous f prefix
(F541)
src/Workflow.py
456-456: f-string without any placeholders
Remove extraneous f prefix
(F541)
470-470: f-string without any placeholders
Remove extraneous f prefix
(F541)
478-478: f-string without any placeholders
Remove extraneous f prefix
(F541)
489-489: f-string without any placeholders
Remove extraneous f prefix
(F541)
521-521: Loop control variable method not used within loop body
(B007)
521-521: Loop control variable secondary not used within loop body
(B007)
542-542: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
| heatmap = simulation_data.loc[:,['monoMasses', 'rt', 'precursorIntensity']] | ||
| heatmap = heatmap.reset_index() | ||
| heatmap = simulation_data.rename(columns={ | ||
| 'monoMasses' : 'mass', 'precursorIntensity' : 'intensity', | ||
| 'index' : 'scan_idx' | ||
| }) |
There was a problem hiding this comment.
Fix logic error and formatting issues.
There's a critical logic error on line 19 where simulation_data is used instead of heatmap, and several formatting issues need correction.
- heatmap = simulation_data.loc[:,['monoMasses', 'rt', 'precursorIntensity']]
+ heatmap = simulation_data.loc[:, ['monoMasses', 'rt', 'precursorIntensity']]
heatmap = heatmap.reset_index()
- heatmap = simulation_data.rename(columns={
- 'monoMasses' : 'mass', 'precursorIntensity' : 'intensity',
- 'index' : 'scan_idx'
+ heatmap = heatmap.rename(columns={
+ 'monoMasses': 'mass', 'precursorIntensity': 'intensity',
+ 'index': 'scan_idx'
})📝 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.
| heatmap = simulation_data.loc[:,['monoMasses', 'rt', 'precursorIntensity']] | |
| heatmap = heatmap.reset_index() | |
| heatmap = simulation_data.rename(columns={ | |
| 'monoMasses' : 'mass', 'precursorIntensity' : 'intensity', | |
| 'index' : 'scan_idx' | |
| }) | |
| heatmap = simulation_data.loc[:, ['monoMasses', 'rt', 'precursorIntensity']] | |
| heatmap = heatmap.reset_index() | |
| heatmap = heatmap.rename(columns={ | |
| 'monoMasses': 'mass', 'precursorIntensity': 'intensity', | |
| 'index': 'scan_idx' | |
| }) |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 17-17: missing whitespace after ','
(E231)
[error] 20-20: whitespace before ':'
(E203)
[error] 20-20: whitespace before ':'
(E203)
[error] 21-21: whitespace before ':'
(E203)
🤖 Prompt for AI Agents
In src/parse/ida.py around lines 17 to 22, replace the use of simulation_data
with heatmap on line 19 to correctly rename columns on the heatmap DataFrame.
Also, fix formatting issues by ensuring consistent indentation and spacing in
the code block to improve readability and maintainability.
| import pandas as pd | ||
| import numpy as np | ||
|
|
||
| from src.parse.masstable import parseFLASHDeconvOutput, getMSSignalDF, getSpectraTableDF | ||
| from src.render.compression import downsample_heatmap, compute_compression_levels | ||
| from scipy.stats import gaussian_kde |
There was a problem hiding this comment.
Remove unused imports.
Multiple imports are not used in this module and should be removed to clean up the code.
-import numpy as np
-
-from src.parse.masstable import parseFLASHDeconvOutput, getMSSignalDF, getSpectraTableDF
+from src.parse.masstable import parseFLASHDeconvOutput, getMSSignalDF, getSpectraTableDF
-from scipy.stats import gaussian_kdeCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 2-2: 'numpy as np' imported but unused
(F401)
[error] 4-4: 'src.parse.masstable.parseFLASHDeconvOutput' imported but unused
(F401)
[error] 4-4: 'src.parse.masstable.getMSSignalDF' imported but unused
(F401)
[error] 4-4: 'src.parse.masstable.getSpectraTableDF' imported but unused
(F401)
[error] 6-6: 'scipy.stats.gaussian_kde' imported but unused
(F401)
🪛 Ruff (0.11.9)
2-2: numpy imported but unused
Remove unused import: numpy
(F401)
4-4: src.parse.masstable.parseFLASHDeconvOutput imported but unused
Remove unused import
(F401)
4-4: src.parse.masstable.getMSSignalDF imported but unused
Remove unused import
(F401)
4-4: src.parse.masstable.getSpectraTableDF imported but unused
Remove unused import
(F401)
6-6: scipy.stats.gaussian_kde imported but unused
Remove unused import: scipy.stats.gaussian_kde
(F401)
🤖 Prompt for AI Agents
In src/parse/ida.py at the beginning of the file (lines 1 to 6), some imported
modules and functions are not used anywhere in the code. Review all the imports
and remove those that are not referenced in the file to clean up and optimize
the code.
src/parse/ida.py
Outdated
| def parseIda( | ||
| file_manager, dataset_id, out_simulation, logger=None | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper spacing and documentation.
The function needs proper spacing and a docstring to explain its purpose and parameters.
+
def parseIda(
file_manager, dataset_id, out_simulation, logger=None
):
+ """
+ Parse FLASHIda simulation results and create compressed heatmaps.
+
+ Args:
+ file_manager: FileManager instance for storing data
+ dataset_id (str): Unique identifier for the dataset
+ out_simulation (str): Path to simulation results TSV file
+ logger: Optional logger for progress messages
+ """📝 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.
| def parseIda( | |
| file_manager, dataset_id, out_simulation, logger=None | |
| ): | |
| def parseIda( | |
| file_manager, dataset_id, out_simulation, logger=None | |
| ): | |
| """ | |
| Parse FLASHIda simulation results and create compressed heatmaps. | |
| Args: | |
| file_manager: FileManager instance for storing data | |
| dataset_id (str): Unique identifier for the dataset | |
| out_simulation (str): Path to simulation results TSV file | |
| logger: Optional logger for progress messages | |
| """ | |
| # existing implementation follows... |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 8-8: expected 2 blank lines, found 1
(E302)
🤖 Prompt for AI Agents
In src/parse/ida.py around lines 8 to 10, the function definition lacks proper
spacing and a docstring. Add a blank line before the function definition for
readability and include a docstring immediately after the function signature
that clearly describes the function's purpose, its parameters (file_manager,
dataset_id, out_simulation, logger), and any return values or exceptions it may
raise.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
scripts/flashida/write_method.py (4)
29-29: ** Fix DEFAULTS structure inconsistency.**The DEFAULTS list still contains string literals ('MS1', 'MS2', 'IDA') mixed with dictionary entries, which creates inconsistent structure. This was flagged in previous reviews but remains unaddressed.
These string literals should be converted to dictionary entries with a clear section marker, for example:
- 'MS1', + {'key': 'MS1', 'section': True},Also applies to: 40-40, 50-50
51-51: ** Fix missing whitespace after comma.**Missing whitespace after comma in the dictionary entry, which was flagged in previous reviews.
- {'key': 'MaxMs2CountPerMs1', 'name': 'MaxMs2CountPerMs1','value': 4}, + {'key': 'MaxMs2CountPerMs1', 'name': 'MaxMs2CountPerMs1', 'value': 4},
68-74: ** Add proper function spacing and improve robustness.**The get_params function lacks proper spacing and error handling as noted in previous reviews.
+ def get_params(): if len(sys.argv) > 1: - with open(sys.argv[1], "r") as f: - return json.load(f) + try: + with open(sys.argv[1], "r") as f: + return json.load(f) + except (FileNotFoundError, json.JSONDecodeError) as e: + print(f"Error reading parameter file: {e}") + return {} else: return {} +
75-79: ** Add proper spacing and error handling for main execution.**The main execution block needs better spacing and error handling for missing keys as noted in previous reviews.
+ if __name__ == "__main__": params = get_params() + - input_xml = params.pop('input_xml') + if 'input_xml' not in params: + raise ValueError("Missing required 'input_xml' parameter") + input_xml = params.pop('input_xml')src/Workflow.py (2)
556-578: ** Add docstring to_find_rawsmethod.**The
_find_rawsmethod lacks documentation as mentioned in previous reviews.def _find_raws(self, raw_path): + """ + Scans a directory for raw files matching the FLASHIda pattern. + + Expected filename format: *FLASHIda_METHOD[_SUFFIX].raw + where METHOD is the name of the method file (without .xml extension) + and optional SUFFIX of '2' indicates using secondary executable. + + Args: + raw_path (Path): Directory path to scan for raw files + + Returns: + tuple: (list of raw filenames, list of method names, list of secondary flags) + """ # Find existing raw files
515-517: ** Add graceful shutdown to the infinite loop.**The infinite loop runs indefinitely without any exit mechanism, which was flagged in previous reviews. This can cause issues when the Streamlit app is closed.
+ self._running = True self.logger.log('Listening for new raw files...') - while(True): + while self._running: # Scan every 1s sleep(1)Also add a stop method to the class:
+ def stop(self): + """Stop the workflow processing loop.""" + self._running = False
🧹 Nitpick comments (8)
content/FLASHIda/FLASHIdaViewer.py (3)
13-13: Remove debug print statements.Debug print statements should be removed from production code.
- print(st.session_state.selected_experiment0_ida)- print('Lets go!')Also applies to: 97-97
74-74: Remove extraneous f-string prefixes.f-strings without placeholders should use regular strings.
- key=f'selected_experiment_dropdown_1_ida', + key='selected_experiment_dropdown_1_ida',- if f"selected_experiment1_ida" in st.session_state: + if "selected_experiment1_ida" in st.session_state:Also applies to: 78-78
106-107: Fix unused loop variable and multiple statements.The loop variable
exp_layoutis not used and there are multiple statements on one line.- for exp_index, exp_layout in enumerate(layout): - if exp_index == 0: continue # skip the first experiment + for exp_index, _ in enumerate(layout): + if exp_index == 0: + continue # skip the first experimentsrc/Workflow.py (5)
456-456: Remove extraneous f-string prefixes.f-strings without placeholders should use regular strings.
- self.logger.log(f'Found FLASHIda executable!') + self.logger.log('Found FLASHIda executable!')- self.logger.log(f'Found Secondary FLASHIda executable!') + self.logger.log('Found Secondary FLASHIda executable!')- self.logger.log(f'Found methods folder!') + self.logger.log('Found methods folder!')- self.logger.log(f'Found raw file folder!') + self.logger.log('Found raw file folder!')- self.logger.log(f'Starting FLASHDeconv...') + self.logger.log('Starting FLASHDeconv...')Also applies to: 470-470, 478-478, 489-489, 542-542
521-523: Fix unused loop variables.The loop variables
methodandsecondaryare not used within the loop body.- for raw, method, secondary in zip(new_raws, new_methods, new_secs): + for raw, _, _ in zip(new_raws, new_methods, new_secs): if raw not in ign_raws: break
537-538: Remove debug print statements.Debug print statements should be removed from production code.
- self.logger.log(secondary) - self.logger.log(flashida_secondary_path)
615-615: Remove unused variable.The variable
dataset_idis assigned but never used, which was flagged in previous reviews.- dataset_id = '%s_%s'%(current_base, current_time)
649-649: Fix spacing around keyword arguments.There are unexpected spaces around the
cwdparameter.- cwd = Path(params['executable']).parent + cwd=Path(params['executable']).parent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
content/FLASHIda/FLASHIdaViewer.py(1 hunks)scripts/flashida/write_method.py(1 hunks)src/Workflow.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
content/FLASHIda/FLASHIdaViewer.py
74-74: f-string without any placeholders
Remove extraneous f prefix
(F541)
78-78: f-string without any placeholders
Remove extraneous f prefix
(F541)
106-106: Loop control variable exp_layout not used within loop body
Rename unused exp_layout to _exp_layout
(B007)
107-107: Multiple statements on one line (colon)
(E701)
src/Workflow.py
456-456: f-string without any placeholders
Remove extraneous f prefix
(F541)
470-470: f-string without any placeholders
Remove extraneous f prefix
(F541)
478-478: f-string without any placeholders
Remove extraneous f prefix
(F541)
489-489: f-string without any placeholders
Remove extraneous f prefix
(F541)
521-521: Loop control variable method not used within loop body
(B007)
521-521: Loop control variable secondary not used within loop body
(B007)
542-542: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Gitleaks (8.26.0)
scripts/flashida/write_method.py
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
| st.selectbox( | ||
| "choose experiment", results, | ||
| key="selected_experiment_dropdown_ida", | ||
| index=validate_selected_index(file_manager, 'selected_experiment0_ida'), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function calls to pass required parameters.
Since the helper functions now need additional parameters, update the function calls accordingly.
- index=validate_selected_index(file_manager, 'selected_experiment0_ida'),
- on_change=select_experiment
+ index=validate_selected_index(file_manager, 'selected_experiment0_ida', name_to_index),
+ on_change=lambda: select_experiment(layout)Apply similar changes to other function calls.
Also applies to: 75-75, 91-91, 114-114
🤖 Prompt for AI Agents
In content/FLASHIda/FLASHIdaViewer.py at lines 63, 75, 91, and 114, the function
calls to helper functions like validate_selected_index need to be updated to
include the new required parameters. Review the function definitions to identify
all required parameters and modify each call to pass these parameters
accordingly, ensuring all calls match the updated function signatures.
| def validate_selected_index(file_manager, selected_experiment): | ||
| results = file_manager.get_results_list(['simulation_dfs']) | ||
| if selected_experiment in st.session_state: | ||
| if st.session_state[selected_experiment] in results: | ||
| return name_to_index[st.session_state[selected_experiment]] | ||
| else: | ||
| del st.session_state[selected_experiment] | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix global variable dependency in validation function.
The validate_selected_index function references name_to_index from the global scope, which makes it non-reusable.
-def validate_selected_index(file_manager, selected_experiment):
+def validate_selected_index(file_manager, selected_experiment, name_to_index):
results = file_manager.get_results_list(['simulation_dfs'])
if selected_experiment in st.session_state:
if st.session_state[selected_experiment] in results:
return name_to_index[st.session_state[selected_experiment]]
else:
del st.session_state[selected_experiment]
return None🤖 Prompt for AI Agents
In content/FLASHIda/FLASHIdaViewer.py around lines 20 to 27, the function
validate_selected_index uses the global variable name_to_index, making it
non-reusable. To fix this, add name_to_index as a parameter to the function
signature and pass it explicitly when calling the function, removing the
dependency on the global scope.
| def select_experiment(): | ||
| st.session_state.selected_experiment0_ida = st.session_state.selected_experiment_dropdown_ida | ||
| print(st.session_state.selected_experiment0_ida) | ||
| if len(layout) > 1: | ||
| for exp_index in range(1, len(layout)): | ||
| if st.session_state[f'selected_experiment_dropdown_{exp_index}_ida'] is None: | ||
| continue | ||
| st.session_state[f"selected_experiment{exp_index}_ida"] = st.session_state[f'selected_experiment_dropdown_{exp_index}_ida'] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix global variable dependencies in function.
The select_experiment function references layout and other variables from the global scope, which makes it non-reusable and harder to test.
-def select_experiment():
+def select_experiment(layout):
st.session_state.selected_experiment0_ida = st.session_state.selected_experiment_dropdown_ida
- print(st.session_state.selected_experiment0_ida)
if len(layout) > 1:
for exp_index in range(1, len(layout)):
if st.session_state[f'selected_experiment_dropdown_{exp_index}_ida'] is None:
continue
st.session_state[f"selected_experiment{exp_index}_ida"] = st.session_state[f'selected_experiment_dropdown_{exp_index}_ida']📝 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.
| def select_experiment(): | |
| st.session_state.selected_experiment0_ida = st.session_state.selected_experiment_dropdown_ida | |
| print(st.session_state.selected_experiment0_ida) | |
| if len(layout) > 1: | |
| for exp_index in range(1, len(layout)): | |
| if st.session_state[f'selected_experiment_dropdown_{exp_index}_ida'] is None: | |
| continue | |
| st.session_state[f"selected_experiment{exp_index}_ida"] = st.session_state[f'selected_experiment_dropdown_{exp_index}_ida'] | |
| def select_experiment(layout): | |
| st.session_state.selected_experiment0_ida = st.session_state.selected_experiment_dropdown_ida | |
| if len(layout) > 1: | |
| for exp_index in range(1, len(layout)): | |
| if st.session_state[f'selected_experiment_dropdown_{exp_index}_ida'] is None: | |
| continue | |
| st.session_state[f"selected_experiment{exp_index}_ida"] = st.session_state[f'selected_experiment_dropdown_{exp_index}_ida'] |
🤖 Prompt for AI Agents
In content/FLASHIda/FLASHIdaViewer.py around lines 11 to 19, the function
select_experiment uses the global variable layout and session_state keys
directly, making it dependent on external state. Refactor the function to accept
layout and session_state as parameters instead of accessing them globally.
Update all references inside the function to use these parameters, which will
improve reusability and testability by removing hidden dependencies.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Workflow.py (2)
515-517: Infinite loop still lacks graceful termination mechanism.This addresses the unresolved issue from previous reviews. The infinite loop with
while(True): sleep(1)has no way to terminate when the Streamlit app is closed.Add a termination mechanism:
+ self._running = True self.logger.log('Listening for new raw files...') - while(True): + while self._running: # Scan every 1s sleep(1)Also add a stop method to the class:
+ def stop(self): + """Stop the workflow execution loop.""" + self._running = False
554-576: Add docstring to_find_rawsmethod.The method performs complex regex pattern matching but lacks documentation explaining the file naming convention.
def _find_raws(self, raw_path): + """ + Scans a directory for raw files matching the FLASHIda pattern. + + Expected filename format: *FLASHIda_METHOD[_SUFFIX].raw + where METHOD is the name of the method file (without .xml extension) + and optional SUFFIX of '2' indicates using secondary executable. + + Args: + raw_path (Path): Directory path to scan for raw files + + Returns: + tuple: (list of raw filenames, list of method names, list of secondary flags) + """
🧹 Nitpick comments (2)
src/Workflow.py (2)
456-456: Remove unnecessary f-string prefixes.Several log messages use f-strings without placeholders, which is unnecessary.
- self.logger.log(f'Found FLASHIda executable!') + self.logger.log('Found FLASHIda executable!') - self.logger.log(f'Found Secondary FLASHIda executable!') + self.logger.log('Found Secondary FLASHIda executable!') - self.logger.log(f'Found methods folder!') + self.logger.log('Found methods folder!') - self.logger.log(f'Found raw file folder!') + self.logger.log('Found raw file folder!') - self.logger.log(f'Starting FLASHDeconv...') + self.logger.log('Starting FLASHDeconv...')Also applies to: 470-470, 478-478, 489-489, 540-540
614-615: Verify consistent formatting with existing code.The string formatting should be consistent with the rest of the codebase.
- dataset_id = '%s_%s'%(current_base, current_time) - folder_path = join(base_path, 'FLASHIdaOutput', '%s_%s'%(current_base, current_time)) + dataset_id = '%s_%s' % (current_base, current_time) + folder_path = join(base_path, 'FLASHIdaOutput', '%s_%s' % (current_base, current_time))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Workflow.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
src/Workflow.py
456-456: f-string without any placeholders
Remove extraneous f prefix
(F541)
470-470: f-string without any placeholders
Remove extraneous f prefix
(F541)
478-478: f-string without any placeholders
Remove extraneous f prefix
(F541)
489-489: f-string without any placeholders
Remove extraneous f prefix
(F541)
521-521: Loop control variable method not used within loop body
(B007)
521-521: Loop control variable secondary not used within loop body
(B007)
540-540: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (7)
src/Workflow.py (7)
1-1: New imports look appropriate and are used in the code.The added imports (
re,pyopenms,sleep, additional path operations, andparseIda) are all utilized in the new workflow classes.Also applies to: 7-7, 9-9, 13-13, 17-17
418-425: Constructor properly initializes the IdaWorkflow.The initialization follows the established pattern with appropriate workflow name and script path configuration.
426-442: Configuration method sets up required input widgets.The four input widgets for executable paths and folder paths are properly configured for the workflow's needs.
580-587: IdaSimulatorWorkflow constructor properly initialized.The constructor correctly sets up the workflow name and script path for the simulation workflow.
588-590: Upload method correctly configured.The mzML file upload widget is properly set up for the simulator workflow.
591-600: Configuration method properly set up.The configuration includes dataset selection, executable path input, and Python script configuration as expected.
602-669: Execution method implements complete simulation workflow.The method properly handles mzML conversion, method generation, simulator execution, and result processing. The logic flow is sound and follows established patterns.
| for raw, method, secondary in zip(new_raws, new_methods, new_secs): | ||
| if raw not in ign_raws: | ||
| break |
There was a problem hiding this comment.
Fix unused loop control variables.
The loop variables method and secondary are not used within the loop body, but they should be for proper logic flow.
for raw, method, secondary in zip(new_raws, new_methods, new_secs):
if raw not in ign_raws:
+ # Found a new raw file with its method and secondary flag
breakCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.11.9)
521-521: Loop control variable method not used within loop body
(B007)
521-521: Loop control variable secondary not used within loop body
(B007)
🤖 Prompt for AI Agents
In src/Workflow.py around lines 521 to 523, the loop variables 'method' and
'secondary' are declared but not used inside the loop, which is incorrect.
Review the loop logic and incorporate 'method' and 'secondary' variables
appropriately within the loop body to ensure they contribute to the intended
processing or conditions. If they are not needed, remove them from the loop
declaration to avoid unused variables.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
content/FLASHIda/FLASHIdaViewer.py (2)
18-25: Remove hidden global dependencies in callbacks; pass layout explicitly.Pass layout into select_experiment via on_change to eliminate reliance on a global. Update all call sites.
-def select_experiment(): +def select_experiment(layout): st.session_state.selected_experiment0_ida = st.session_state.selected_experiment_dropdown_ida if len(layout) > 1: for exp_index in range(1, len(layout)): if st.session_state[f'selected_experiment_dropdown_{exp_index}_ida'] is None: continue st.session_state[f"selected_experiment{exp_index}_ida"] = st.session_state[f'selected_experiment_dropdown_{exp_index}_ida']- index=validate_selected_index(file_manager, 'selected_experiment0_ida'), - on_change=select_experiment + index=validate_selected_index(file_manager, 'selected_experiment0_ida', name_to_index), + on_change=lambda: select_experiment(layout)- index=validate_selected_index(file_manager, 'selected_experiment1_ida'), - on_change=select_experiment + index=validate_selected_index(file_manager, 'selected_experiment1_ida', name_to_index), + on_change=lambda: select_experiment(layout)- index=validate_selected_index(file_manager, 'selected_experiment0_ida'), - on_change=select_experiment + index=validate_selected_index(file_manager, 'selected_experiment0_ida', name_to_index), + on_change=lambda: select_experiment(layout)- index=validate_selected_index(file_manager, f'selected_experiment{exp_index}_ida'), - on_change=select_experiment + index=validate_selected_index(file_manager, f'selected_experiment{exp_index}_ida', name_to_index), + on_change=lambda: select_experiment(layout)Also applies to: 72-74, 81-83, 93-95, 111-113
26-35: Eliminate global name_to_index usage in validate_selected_index.Pass name_to_index as a parameter to avoid global state coupling.
-def validate_selected_index(file_manager, selected_experiment): +def validate_selected_index(file_manager, selected_experiment, name_to_index): results = file_manager.get_results_list( ['deconv_dfs', 'anno_dfs', 'ms2_dfs'] ) if selected_experiment in st.session_state: if st.session_state[selected_experiment] in results: - return name_to_index[st.session_state[selected_experiment]] + return name_to_index[st.session_state[selected_experiment]] else: del st.session_state[selected_experiment] return Nonesrc/parse/ida.py (3)
1-7: Remove unused importsClean up unused imports to satisfy F401 and reduce module load time.
-import pandas as pd -import numpy as np +import pandas as pd - -from src.parse.masstable import parseFLASHDeconvOutput, getMSSignalDF, getSpectraTableDF from src.render.compression import downsample_heatmap, compute_compression_levels -from scipy.stats import gaussian_kde
8-16: Add docstrings for clarity and spacingAdd concise docstrings describing inputs/outputs and purpose. Also ensure proper spacing per E302.
def parseIdaSimulator( file_manager, dataset_id, out_simulation, logger=None ): + """ + Parse FLASHIda simulator TSV output. + - Stores raw simulation DataFrame under 'simulation_dfs' + - Builds and stores an MS1 deconv heatmap and compressed variants + """ @@ def parseIda( file_manager, dataset_id, spec2_tsv, id_tsv ): + """ + Parse FLASHIda identification and scan TSV files. + - Marks MS2 scans as identified based on ID table (TopPIC) + - Stores resulting DataFrame under 'ms2_dfs' + """Also applies to: 40-49
17-22: Fix logic error: rename columns on the correct DataFrameColumns should be renamed on the sliced heatmap, not on simulation_data. Also fix spacing.
- heatmap = simulation_data.loc[:,['monoMasses', 'rt', 'precursorIntensity']] + heatmap = simulation_data.loc[:, ['monoMasses', 'rt', 'precursorIntensity']] heatmap = heatmap.reset_index() - heatmap = simulation_data.rename(columns={ - 'monoMasses' : 'mass', 'precursorIntensity' : 'intensity', - 'index' : 'scan_idx' - }) + heatmap = heatmap.rename(columns={ + 'monoMasses': 'mass', + 'precursorIntensity': 'intensity', + 'index': 'scan_idx', + })src/Workflow.py (1)
515-518: Add graceful shutdown to infinite loopCurrently loops forever with sleep(1). Introduce a running flag/event so the workflow can stop cleanly when the app exits.
Proposed minimal changes:
- In IdaWorkflow.init: set self._running = True
- In execution: while self._running:
- Add a stop(self) method to flip the flag
- Ensure the caller (manager) invokes stop() on teardown
I can generate a full patch wired to WorkflowManager if desired.
🧹 Nitpick comments (14)
src/render/initialize.py (1)
124-131: ms2_table keys: confirm id_table schema matches FE expectations.You send per_scan_data + id_table. Ensure FE Tabulator for MS2 expects id_table (not e.g. ms2_table) to avoid runtime key errors.
content/FLASHIda/FLASHIdaViewer.py (2)
80-80: Remove unnecessary f-strings.No interpolation here; drop the f prefix.
- key=f'selected_experiment_dropdown_1_ida', + key='selected_experiment_dropdown_1_ida',- if f"selected_experiment1_ida" in st.session_state: + if "selected_experiment1_ida" in st.session_state:Also applies to: 84-84
103-104: Address lint: unused loop variable and E701.Rename the unused variable and split the one-liner for readability.
- for exp_index, exp_layout in enumerate(layout): - if exp_index == 0: continue # skip the first experiment + for exp_index, _exp_layout in enumerate(layout): + if exp_index == 0: + continue # skip the first experimentcontent/FLASHIda/FLASHIdaSimulatorViewer.py (4)
74-75: Remove f-strings without placeholdersMinor cleanup to satisfy linters.
- key=f'selected_experiment_dropdown_1_ida', + key='selected_experiment_dropdown_1_ida', ... - if f"selected_experiment1_ida" in st.session_state: + if "selected_experiment1_ida" in st.session_state:Also applies to: 78-78
106-109: Address lint: unused loop variable and one-line if/continueRename the unused variable and split the one-liner to improve readability and satisfy B007/E701.
- for exp_index, exp_layout in enumerate(layout): - if exp_index == 0: continue # skip the first experiment + for exp_index, _exp_layout in enumerate(layout): + if exp_index == 0: + continue # skip the first experiment
12-14: Avoid print() in Streamlit appsUse st.write/st.caption or remove debug prints to keep logs clean.
- print(st.session_state.selected_experiment0_ida) + # st.write(st.session_state.selected_experiment0_ida) # Optional debug ... - print('Lets go!') + # st.caption('Loading selection...') # Optional user feedbackAlso applies to: 97-97
20-27: Avoid referencing globals inside helpersvalidate_selected_index relies on global name_to_index; safer to pass it in or compute locally from results to avoid order mismatches.
Would you like a patch to pass name_to_index into validate_selected_index?
src/parse/ida.py (1)
24-27: Remove extraneous f-stringNo placeholders; store under a constant key.
- file_manager.store_data( - dataset_id, f'ms1_deconv_heatmap', heatmap - ) + file_manager.store_data(dataset_id, 'ms1_deconv_heatmap', heatmap)content/FLASHIda/FLASHIdaWorkflow.py (4)
7-7: Remove unused importsave_params is imported but not used.
-from src.common.common import page_setup, save_params +from src.common.common import page_setup
113-115: Use isinstance for type checksAvoid direct type comparisons per E721.
- if type(uploaded_files) != list: + if not isinstance(uploaded_files, list): uploaded_files = [uploaded_files]
69-71: Avoid print() in Streamlit app pathUse logger or st.write/st.warning for user-visible info; print clutters server logs.
- print(input_files) - print(parsed_files) - print(unparsed_files) + # Debugging (optional): + # st.write(dict(input_files=list(input_files), parsed_files=list(parsed_files), unparsed_files=list(unparsed_files)))
74-84: Harden parsing with error handlingWrap parseDeconv/parseIda to avoid failing the whole UI when a single dataset is malformed.
I can provide a patch to add try/except with logging around the parsing loop if you’d like.
src/Workflow.py (2)
456-456: Remove f-strings without placeholders; fix incorrect log message
- Drop unnecessary f- prefixes.
- Replace "Starting FLASHDeconv..." with "Starting FLASHIda..." when launching FLASHIda.
- self.logger.log(f'Found FLASHIda executable!') + self.logger.log('Found FLASHIda executable!') @@ - self.logger.log(f'Found Secondary FLASHIda executable!') + self.logger.log('Found Secondary FLASHIda executable!') @@ - self.logger.log(f'Found methods folder!') + self.logger.log('Found methods folder!') @@ - self.logger.log(f'Found raw file folder!') + self.logger.log('Found raw file folder!') @@ - self.logger.log(f'Starting FLASHDeconv...') + self.logger.log('Starting FLASHIda...')Also applies to: 470-470, 478-478, 489-489, 540-541
521-526: Satisfy B007 by using loop tuple inside the loop bodyReference the tuple inside the loop to avoid "unused loop variable" warnings and keep logic clear.
- for raw, method, secondary in zip(new_raws, new_methods, new_secs): - if raw not in ign_raws: - break + for item in zip(new_raws, new_methods, new_secs): + raw, method, secondary = item + if raw not in ign_raws: + break
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.streamlit/config.toml(1 hunks)app.py(1 hunks)content/FLASHIda/FLASHIdaSimulatorViewer.py(1 hunks)content/FLASHIda/FLASHIdaViewer.py(1 hunks)content/FLASHIda/FLASHIdaWorkflow.py(1 hunks)src/Workflow.py(2 hunks)src/parse/ida.py(1 hunks)src/render/components.py(2 hunks)src/render/initialize.py(2 hunks)src/render/update.py(1 hunks)src/workflow/FileManager.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .streamlit/config.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- app.py
🧰 Additional context used
🪛 Ruff (0.12.2)
content/FLASHIda/FLASHIdaViewer.py
80-80: f-string without any placeholders
Remove extraneous f prefix
(F541)
84-84: f-string without any placeholders
Remove extraneous f prefix
(F541)
103-103: Loop control variable exp_layout not used within loop body
Rename unused exp_layout to _exp_layout
(B007)
104-104: Multiple statements on one line (colon)
(E701)
content/FLASHIda/FLASHIdaSimulatorViewer.py
74-74: f-string without any placeholders
Remove extraneous f prefix
(F541)
78-78: f-string without any placeholders
Remove extraneous f prefix
(F541)
106-106: Loop control variable exp_layout not used within loop body
Rename unused exp_layout to _exp_layout
(B007)
107-107: Multiple statements on one line (colon)
(E701)
src/Workflow.py
456-456: f-string without any placeholders
Remove extraneous f prefix
(F541)
470-470: f-string without any placeholders
Remove extraneous f prefix
(F541)
478-478: f-string without any placeholders
Remove extraneous f prefix
(F541)
489-489: f-string without any placeholders
Remove extraneous f prefix
(F541)
521-521: Loop control variable method not used within loop body
(B007)
521-521: Loop control variable secondary not used within loop body
(B007)
540-540: f-string without any placeholders
Remove extraneous f prefix
(F541)
content/FLASHIda/FLASHIdaWorkflow.py
7-7: src.common.common.save_params imported but unused
Remove unused import: src.common.common.save_params
(F401)
113-113: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
src/parse/ida.py
2-2: numpy imported but unused
Remove unused import: numpy
(F401)
4-4: src.parse.masstable.parseFLASHDeconvOutput imported but unused
Remove unused import
(F401)
4-4: src.parse.masstable.getMSSignalDF imported but unused
Remove unused import
(F401)
4-4: src.parse.masstable.getSpectraTableDF imported but unused
Remove unused import
(F401)
6-6: scipy.stats.gaussian_kde imported but unused
Remove unused import: scipy.stats.gaussian_kde
(F401)
26-26: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (3)
src/render/components.py (1)
49-56: Add a default case for unknown table types and verify MS2Table component mappingPlease ensure the front-end actually implements a
TabulatorFragmentScanTablecomponent for the MS2Table, and guard against invalidtable_typevalues by raising early errors.• File to update:
src/render/components.pyat the end of theif/elifchain inTabulator.__init__
• Confirm in your FE codebase thatTabulatorFragmentScanTableis defined and used for MS2Table (it’s only referenced here so far).
• Add a finalelsebranch to fail fast on unexpected types:elif table_type == 'TagTable': self.title = 'Tag Table' self.componentName = "TabulatorTagTable" + else: + raise ValueError(f"Unknown table_type: {table_type}")src/render/initialize.py (1)
70-75: Conditional title looks good.Nice, keeps the UI copy aligned to the tool context.
src/Workflow.py (1)
445-446: Verify parameter manager usageOther workflows use self.executor.parameter_manager; here you use self.parameter_manager. Ensure IdaWorkflow has a parameter_manager attribute wired correctly.
Would you like me to scan the codebase to confirm and produce a patch if needed?
| DEFAULT_LAYOUT = [['ms1_deconv_heat_map']] | ||
|
|
There was a problem hiding this comment.
Fix component key mismatch: 'ms1_deconv_heat_map' vs 'ms1_deconv_heatmap'
Parser stores data under 'ms1_deconv_heatmap' (no extra underscore). The default layout uses a different key, so the component won’t render.
Apply this diff:
-DEFAULT_LAYOUT = [['ms1_deconv_heat_map']]
+DEFAULT_LAYOUT = [['ms1_deconv_heatmap']]📝 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.
| DEFAULT_LAYOUT = [['ms1_deconv_heat_map']] | |
| DEFAULT_LAYOUT = [['ms1_deconv_heatmap']] |
🤖 Prompt for AI Agents
In content/FLASHIda/FLASHIdaSimulatorViewer.py around lines 9 to 10, the
DEFAULT_LAYOUT uses the key 'ms1_deconv_heat_map' which does not match the
parser's key 'ms1_deconv_heatmap'. Update the DEFAULT_LAYOUT to use
'ms1_deconv_heatmap' without the extra underscore to ensure the component
renders correctly.
| st.selectbox( | ||
| "choose experiment", results, | ||
| key="selected_experiment_dropdown_ida", | ||
| index=validate_selected_index(file_manager, 'selected_experiment0_ida'), | ||
| on_change=select_experiment | ||
| ) | ||
| if 'selected_experiment0_ida' in st.session_state: |
There was a problem hiding this comment.
Selectbox index may be None; Streamlit expects an int
validate_selected_index can return None, which makes selectbox(index=...) raise. Default the index to 0 when None.
- st.selectbox(
+ st.selectbox(
"choose experiment", results,
key="selected_experiment_dropdown_ida",
- index=validate_selected_index(file_manager, 'selected_experiment0_ida'),
+ index=validate_selected_index(file_manager, 'selected_experiment0_ida') or 0,
on_change=select_experiment
)
...
- st.selectbox(
+ st.selectbox(
"choose experiment", results,
- key=f'selected_experiment_dropdown_1_ida',
- index=validate_selected_index(file_manager, 'selected_experiment1_ida'),
+ key='selected_experiment_dropdown_1_ida',
+ index=validate_selected_index(file_manager, 'selected_experiment1_ida') or 0,
on_change=select_experiment
)
...
- st.selectbox(
+ st.selectbox(
"choose experiment", results,
key="selected_experiment_dropdown_ida",
- index=validate_selected_index(file_manager, 'selected_experiment0_ida'),
+ index=validate_selected_index(file_manager, 'selected_experiment0_ida') or 0,
on_change=select_experiment
)
...
- st.selectbox(
+ st.selectbox(
"choose experiment", results,
key=f'selected_experiment_dropdown_{exp_index}_ida',
- index=validate_selected_index(file_manager, f'selected_experiment{exp_index}_ida'),
+ index=validate_selected_index(file_manager, f'selected_experiment{exp_index}_ida') or 0,
on_change=select_experiment
)Also applies to: 72-78, 88-93, 111-116
🤖 Prompt for AI Agents
In content/FLASHIda/FLASHIdaSimulatorViewer.py around lines 60 to 66, the index
argument passed to st.selectbox can be None because validate_selected_index may
return None, but Streamlit requires an integer index. To fix this, ensure that
if validate_selected_index returns None, you default the index to 0 before
passing it to the selectbox. Apply the same fix to the selectbox calls in lines
72-78, 88-93, and 111-116.
| if 'selected_experiment0_ida' in st.session_state: | ||
| print('Lets go!') | ||
| render_grid( | ||
| st.session_state.selected_experiment0_ida, layout[0], file_manager, | ||
| 'flashidasimulator', 'selected_experiment0_ida' | ||
| ) |
There was a problem hiding this comment.
render_grid call missing a stable grid key in single-experiment view
Side-by-side paths pass a grid key; single-view doesn’t. This can cause state/key collisions and inconsistent UI updates.
- render_grid(
- st.session_state.selected_experiment0_ida, layout[0], file_manager,
- 'flashidasimulator', 'selected_experiment0_ida'
- )
+ render_grid(
+ st.session_state.selected_experiment0_ida, layout[0], file_manager,
+ 'flashidasimulator', 'selected_experiment0_ida', 'flash_viewer_grid_0_ida'
+ )📝 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.
| if 'selected_experiment0_ida' in st.session_state: | |
| print('Lets go!') | |
| render_grid( | |
| st.session_state.selected_experiment0_ida, layout[0], file_manager, | |
| 'flashidasimulator', 'selected_experiment0_ida' | |
| ) | |
| if 'selected_experiment0_ida' in st.session_state: | |
| print('Lets go!') | |
| render_grid( | |
| st.session_state.selected_experiment0_ida, layout[0], file_manager, | |
| 'flashidasimulator', 'selected_experiment0_ida', 'flash_viewer_grid_0_ida' | |
| ) |
🤖 Prompt for AI Agents
In content/FLASHIda/FLASHIdaSimulatorViewer.py around lines 96 to 101, the
render_grid call in the single-experiment view is missing a stable grid key,
which can lead to state collisions and inconsistent UI updates. Add a unique and
stable key argument to the render_grid call similar to the side-by-side paths
usage to ensure proper state management and UI consistency.
| | set(wf.file_manager.get_results_list(['ids_tsv'])) | ||
| ) |
There was a problem hiding this comment.
Fix dataset key typo ('ids_tsv' -> 'id_tsv')
Key used elsewhere is 'id_tsv'; the plural form prevents those datasets from appearing in the union set.
- | set(wf.file_manager.get_results_list(['ids_tsv']))
+ | set(wf.file_manager.get_results_list(['id_tsv']))📝 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.
| | set(wf.file_manager.get_results_list(['ids_tsv'])) | |
| ) | |
| | set(wf.file_manager.get_results_list(['id_tsv'])) | |
| ) |
🤖 Prompt for AI Agents
In content/FLASHIda/FLASHIdaWorkflow.py at lines 127 to 128, the dataset key
'ids_tsv' is a typo and should be corrected to 'id_tsv' to match the key used
elsewhere. Replace 'ids_tsv' with 'id_tsv' in the call to get_results_list to
ensure the datasets are correctly included in the union set.
| # Create a _RELEASE constant. We'll set this to False while we're developing | ||
| # the component, and True when we're ready to package and distribute it. | ||
| _RELEASE = True | ||
| _RELEASE = False |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Change _RELEASE via environment, not hardcoded.
Avoid committing development mode. Make it configurable so production builds don't depend on manual edits.
-_RELEASE = False
+_RELEASE = os.environ.get("STREAMLIT_COMPONENT_RELEASE", "0") == "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.
| _RELEASE = False | |
| - _RELEASE = False | |
| + _RELEASE = os.environ.get("STREAMLIT_COMPONENT_RELEASE", "0") == "1" |
🤖 Prompt for AI Agents
In src/render/components.py at line 9, the _RELEASE variable is hardcoded to
False, which is not flexible for different environments. Modify the code to set
_RELEASE based on an environment variable, such as using os.getenv to read a
value like "RELEASE" and convert it to a boolean. This allows toggling release
mode without changing the code manually.
| if (component == 'Augmented Annotated Spectrum' and tool == 'flashida'): | ||
| if 'precursorScanIndex' not in selection_store: | ||
| data['per_scan_data'] = data['per_scan_data'].iloc[0:0,:] | ||
| else: | ||
| data['per_scan_data'] = data['per_scan_data'].iloc[selection_store['precursorScanIndex']:selection_store['precursorScanIndex']+1,:] | ||
| elif component in [ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing/None precursorScanIndex robustly.
Selection callbacks can provide None even when the key exists. Use get() and branch on None to avoid KeyErrors and make intent explicit.
- if (component == 'Augmented Annotated Spectrum' and tool == 'flashida'):
- if 'precursorScanIndex' not in selection_store:
- data['per_scan_data'] = data['per_scan_data'].iloc[0:0,:]
- else:
- data['per_scan_data'] = data['per_scan_data'].iloc[selection_store['precursorScanIndex']:selection_store['precursorScanIndex']+1,:]
+ if (component == 'Augmented Annotated Spectrum' and tool == 'flashida'):
+ idx = selection_store.get('precursorScanIndex')
+ if idx is None:
+ data['per_scan_data'] = data['per_scan_data'].iloc[0:0, :]
+ else:
+ data['per_scan_data'] = data['per_scan_data'].iloc[idx:idx+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.
| if (component == 'Augmented Annotated Spectrum' and tool == 'flashida'): | |
| if 'precursorScanIndex' not in selection_store: | |
| data['per_scan_data'] = data['per_scan_data'].iloc[0:0,:] | |
| else: | |
| data['per_scan_data'] = data['per_scan_data'].iloc[selection_store['precursorScanIndex']:selection_store['precursorScanIndex']+1,:] | |
| elif component in [ | |
| if (component == 'Augmented Annotated Spectrum' and tool == 'flashida'): | |
| idx = selection_store.get('precursorScanIndex') | |
| if idx is None: | |
| data['per_scan_data'] = data['per_scan_data'].iloc[0:0, :] | |
| else: | |
| data['per_scan_data'] = data['per_scan_data'].iloc[idx:idx+1, :] | |
| elif component in [ |
🤖 Prompt for AI Agents
In src/render/update.py around lines 89 to 94, the code checks for
'precursorScanIndex' key presence but does not handle cases where its value is
None. Replace the current key existence check with
selection_store.get('precursorScanIndex') and explicitly check if the returned
value is None. Branch accordingly to handle None values safely and avoid
KeyErrors, making the intent clearer and more robust.
| method_pattern = r'.*FLASHIda_([^.]+)\.raw' | ||
| for file in listdir(raw_path): | ||
| if not Path(raw_path, file).is_file(): | ||
| continue | ||
| match = re.search(method_pattern, file) | ||
| if match: | ||
| raws.append(str(file)) | ||
| full_suffix = match.group(1) | ||
| parts = full_suffix.split('_', 1) | ||
| method = parts[0] | ||
| methods.append(method) | ||
| suffix = parts[1] if len(parts) > 1 else None | ||
| if suffix is not None: | ||
| suffix = suffix.split('_')[0] if '_' in suffix else suffix | ||
| suffix = suffix.split('.')[0] if '.' in suffix else suffix | ||
| secondary = suffix == '2' if suffix else False | ||
| secondary_flags.append(secondary) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make raw filename parsing robust to underscores in method names
Current split('_', 1) mis-parses methods containing underscores. Prefer a regex with an optional trailing "_2" suffix.
- method_pattern = r'.*FLASHIda_([^.]+)\.raw'
- for file in listdir(raw_path):
+ method_pattern = r'.*FLASHIda_(?P<method>.+?)(?:_(?P<suffix>2))?\.raw$'
+ for file in listdir(raw_path):
if not Path(raw_path, file).is_file():
continue
- match = re.search(method_pattern, file)
+ match = re.match(method_pattern, file)
if match:
- raws.append(str(file))
- full_suffix = match.group(1)
- parts = full_suffix.split('_', 1)
- method = parts[0]
- methods.append(method)
- suffix = parts[1] if len(parts) > 1 else None
- if suffix is not None:
- suffix = suffix.split('_')[0] if '_' in suffix else suffix
- suffix = suffix.split('.')[0] if '.' in suffix else suffix
- secondary = suffix == '2' if suffix else False
- secondary_flags.append(secondary)
+ raws.append(str(file))
+ method = match.group('method')
+ suffix = match.group('suffix')
+ methods.append(method)
+ secondary_flags.append(suffix == '2')If you prefer to preserve exact behavior, I can adjust the pattern accordingly.
📝 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.
| method_pattern = r'.*FLASHIda_([^.]+)\.raw' | |
| for file in listdir(raw_path): | |
| if not Path(raw_path, file).is_file(): | |
| continue | |
| match = re.search(method_pattern, file) | |
| if match: | |
| raws.append(str(file)) | |
| full_suffix = match.group(1) | |
| parts = full_suffix.split('_', 1) | |
| method = parts[0] | |
| methods.append(method) | |
| suffix = parts[1] if len(parts) > 1 else None | |
| if suffix is not None: | |
| suffix = suffix.split('_')[0] if '_' in suffix else suffix | |
| suffix = suffix.split('.')[0] if '.' in suffix else suffix | |
| secondary = suffix == '2' if suffix else False | |
| secondary_flags.append(secondary) | |
| method_pattern = r'.*FLASHIda_(?P<method>.+?)(?:_(?P<suffix>2))?\.raw$' | |
| for file in listdir(raw_path): | |
| if not Path(raw_path, file).is_file(): | |
| continue | |
| match = re.match(method_pattern, file) | |
| if match: | |
| raws.append(str(file)) | |
| method = match.group('method') | |
| suffix = match.group('suffix') | |
| methods.append(method) | |
| secondary_flags.append(suffix == '2') |
🤖 Prompt for AI Agents
In src/Workflow.py around lines 559 to 575, the current filename parsing uses
split('_', 1) which fails for method names containing underscores. To fix this,
update the regex pattern to capture the method name including underscores and
optionally a trailing "_2" suffix. Then extract the method and secondary flag
directly from the regex groups instead of splitting the suffix. This will make
the parsing robust to underscores in method names while preserving the detection
of the secondary suffix.
| name_tags_available = [n for n in name_tags if n in available_columns] | ||
| if not partial and len(name_tags) != len(name_tags_available): | ||
| return [] | ||
| name_tags = name_tags_available | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
get_results_list: guard against string input for name_tags.
Iterating a str splits into characters, breaking filtering. Coerce to list for safety.
- name_tags_available = [n for n in name_tags if n in available_columns]
+ # Coerce to list to avoid iterating over characters when a single tag string is passed
+ if isinstance(name_tags, str):
+ name_tags = [name_tags]
+ name_tags_available = [n for n in name_tags if n in available_columns]
if not partial and len(name_tags) != len(name_tags_available):
return []
name_tags = name_tags_availableI can add a small test to cover str vs list inputs—want me to open a PR with tests?
📝 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.
| name_tags_available = [n for n in name_tags if n in available_columns] | |
| if not partial and len(name_tags) != len(name_tags_available): | |
| return [] | |
| name_tags = name_tags_available | |
| # Coerce to list to avoid iterating over characters when a single tag string is passed | |
| if isinstance(name_tags, str): | |
| name_tags = [name_tags] | |
| name_tags_available = [n for n in name_tags if n in available_columns] | |
| if not partial and len(name_tags) != len(name_tags_available): | |
| return [] | |
| name_tags = name_tags_available |
🤖 Prompt for AI Agents
In src/workflow/FileManager.py around lines 383 to 387, the code iterates over
name_tags assuming it is a list, but if name_tags is a string, it will iterate
over characters causing incorrect filtering. To fix this, add a check to coerce
name_tags to a list if it is a string before the list comprehension. This
ensures consistent behavior regardless of input type.
Summary by CodeRabbit
New Features
Bug Fixes