[FIX] update cli for preprocess #1190
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: The pull request introduces enhancements to the command-line interface for preprocessing tasks. It includes updates to logging within the bidspm module to provide more information about the execution environment, specifically the Matlab and Octave platforms. Additionally, a new shell script is added to facilitate running preprocessing tasks from the command line.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
General suggestions:
- Consider storing the result of the
matlab()function call in a variable to avoid calling it multiple times, which could be a potential performance issue. - For consistency and maintainability, it would be beneficial to log the path to the Octave executable in the same manner as the Matlab path is logged.
- Verify that the participant labels are handled correctly in all contexts, particularly with regard to potential octal interpretation due to leading zeros.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| @@ -429,9 +429,11 @@ def run_command(cmd: str, platform: str | None = None) -> int: | |||
|
|
|||
| if platform is None: | |||
| if Path(matlab()).exists(): | |||
| log.info(f"Using matlab found at: {matlab()}") | |||
There was a problem hiding this comment.
suggestion (llm): Logging the path to the Matlab executable is a good practice for debugging. However, calling matlab() function twice (once for logging and once for setting platform) could be inefficient if the function does any non-trivial computation or IO. Consider storing the result in a variable and reusing it.
| platform = matlab() | ||
| cmd = cmd.replace(new_line, ", ") | ||
| else: | ||
| log.info("Using octave") |
There was a problem hiding this comment.
suggestion (llm): Consistency in logging is important for maintainability. Consider adding the path to the octave executable, similar to how the path to Matlab is logged.
demos/openneuro/ds000001_run.sh
Outdated
| --action preprocess \ | ||
| --task balloonanalogrisktask \ | ||
| --fwhm 8 \ | ||
| --participant_label 01 02 03 |
There was a problem hiding this comment.
question (llm): Ensure that the participant labels are correctly formatted and that leading zeros do not imply octal interpretation in any context where these values are used.
No description provided.