Split autoreduction script into functions and add tests#98
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #98 +/- ##
========================================
Coverage 98.98% 98.99%
========================================
Files 8 11 +3
Lines 591 694 +103
========================================
+ Hits 585 687 +102
- Misses 6 7 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR introduces a new autoreduction workflow for the REF_L instrument. It adds a new autoreduction script ( Sequence DiagramsequenceDiagram
participant User
participant CLI as reduce_REF_L Script
participant Mantid
participant Workflow as Reduction Workflow
participant Plotter as Plot Publisher
participant DataConfirm as Data Confirmation
User->>CLI: Run with events_file, output_dir, params
CLI->>CLI: parse_command_arguments()
CLI->>Mantid: Load event data (events_file)
Mantid-->>CLI: workspace
CLI->>CLI: Select template (default or provided)
CLI->>Workflow: Execute reduction workflow
Workflow->>Mantid: Process reduction
Mantid-->>Workflow: Reduced data
Workflow-->>CLI: Reduced workspace
rect rgb(200, 220, 255)
note over CLI,Plotter: Conditional on publish flag
CLI->>Plotter: upload_report() with reduced data
Plotter->>Plotter: Compute plots and format
Plotter-->>CLI: Reports published
end
CLI->>DataConfirm: confirm_data_availability()
DataConfirm-->>CLI: Confirmation complete/logged
CLI-->>User: Autoreduction workflow complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/lr_reduction/template.py (1)
62-62: Consider defining a custom exception or error message constant.The error message could be extracted to a module-level constant or a custom exception class for better maintainability and testability, though the current implementation is acceptable.
Apply this diff to extract the error message:
+# Module-level constant +NO_TEMPLATE_ERROR_MSG = "No template found: place a template in shared/autoreduce" + def get_default_template_file(output_dir: str, tthd: float) -> str: ... if template_file is None: - raise ValueError("No template found: place a template in shared/autoreduce") + raise ValueError(NO_TEMPLATE_ERROR_MSG)src/lr_reduction/mantid_utils.py (1)
26-31: Consider expanding scalar type checking for robustness.The current type check on Line 28 covers
int,float, andstr, which are the common scalar types. However, Mantid properties might also return boolean values or NumPy scalar types. Consider whether the type check should be expanded or if the current implementation is sufficient for the expected use cases.If needed, you could expand the type check:
- if isinstance(value, (int, float, str)): # scalar sample logs can only be one of these three types + if isinstance(value, (int, float, str, bool)) or hasattr(value, 'ndim') and value.ndim == 0: return valueOr use a simpler approach by checking if the value is not a sequence:
- if isinstance(value, (int, float, str)): # scalar sample logs can only be one of these three types - return value - else: - return value[0] # return the first value + try: + return value[0] # Try to get first element for sequences + except (TypeError, IndexError): + return value # Return directly for scalarssrc/lr_autoreduce/reduce_REF_L.py (4)
61-61: Fix ambiguous character in comment.The comment contains an EN DASH (–) instead of a HYPHEN-MINUS (-). Replace it for consistency and to avoid potential encoding issues.
Apply this diff:
- # Existing behavior: optional positional 4–8 parameters + # Existing behavior: optional positional 4-8 parameters
139-139: Consider extracting error message to a constant.For consistency with best practices and easier maintenance, consider extracting the error message to a module-level constant, though the current implementation is acceptable.
Apply this diff:
+# Module-level constants +ERROR_COMBINED_DATA_NOT_FOUND = "Combined data output file not found" + def upload_report(...): ... if not os.path.isfile(default_file_path): - raise ValueError("Combined data output file not found") + raise ValueError(ERROR_COMBINED_DATA_NOT_FOUND)
149-149: Use consistent string formatting.Line 149 uses string concatenation while Line 158 uses f-strings. Consider using f-strings consistently throughout the function for better readability.
Apply this diff:
- logger.notice('run position: ', run_position) + logger.notice(f'run position: {run_position}')
207-207: Consider adding error handling for theta_offset conversion.If the theta_offset argument contains an invalid float value, the conversion will raise a ValueError. Consider adding try-except handling or using argparse's type parameter for validation.
You could use argparse's type parameter for automatic validation:
parser.add_argument("fit_first_peak", nargs="?", default="false") - parser.add_argument("theta_offset", nargs="?", default="0") + parser.add_argument("theta_offset", nargs="?", default="0", type=float)Then remove the manual conversion:
template_file_arg = args.template_file avg_overlap_arg = str_to_bool(args.avg_overlap) const_q_arg = str_to_bool(args.const_q) - theta_offset_arg = float(args.theta_offset) + theta_offset_arg = args.theta_offsetNote: This would change the type from str to float, which might affect compatibility if callers expect string defaults.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/test_and_deploy.yml(1 hunks)pyproject.toml(4 hunks)src/lr_autoreduce/reduce_REF_L.py(1 hunks)src/lr_reduction/mantid_utils.py(1 hunks)src/lr_reduction/template.py(2 hunks)tests/unit/lr_autoreduce/test_reduce_ref_l.py(1 hunks)tests/unit/lr_reduction/test_template.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-27T18:08:12.480Z
Learnt from: backmari
Repo: neutrons/LiquidsReflectometer PR: 66
File: conda.recipe/meta.yaml:27-40
Timestamp: 2025-05-27T18:08:12.480Z
Learning: In conda environment.yml files, there are two distinct sections: 1) "channels" which specifies package repositories/sources (like mantid-ornl, mantid, conda-forge), and 2) "dependencies" which lists the actual packages to install with their version constraints. Don't confuse channel names with package dependencies.
Applied to files:
pyproject.toml
🧬 Code graph analysis (4)
tests/unit/lr_autoreduce/test_reduce_ref_l.py (1)
src/lr_autoreduce/reduce_REF_L.py (3)
confirm_data_availability(176-191)parse_command_arguments(42-70)str_to_bool(194-195)
tests/unit/lr_reduction/test_template.py (1)
src/lr_reduction/template.py (1)
get_default_template_file(37-64)
src/lr_autoreduce/reduce_REF_L.py (3)
src/lr_reduction/mantid_utils.py (1)
SampleLogs(5-39)src/lr_reduction/template.py (1)
get_default_template_file(37-64)tests/unit/lr_autoreduce/test_reduce_ref_l.py (1)
sample_logs(76-78)
src/lr_reduction/mantid_utils.py (1)
src/lr_reduction/utils.py (1)
workspace_handle(23-39)
🪛 Ruff (0.14.8)
src/lr_reduction/template.py
62-62: Avoid specifying long messages outside the exception class
(TRY003)
src/lr_autoreduce/reduce_REF_L.py
61-61: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
73-73: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
185-185: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (20)
tests/unit/lr_reduction/test_template.py (1)
6-27: LGTM!The test comprehensively covers all code paths: the error case when no templates exist, the fallback to default template, and both geometry-specific templates (up and down). The test structure is clear and the assertions are correct.
.github/workflows/test_and_deploy.yml (1)
118-118: LGTM!Adding the neutrons channel is consistent with the pyproject.toml changes and ensures the conda-verify job can access packages from the neutrons channel during installation.
pyproject.toml (4)
45-45: LGTM!Adding "src/lr_autoreduce" to the packages list is necessary to include the new autoreduction module in the wheel distribution.
73-73: LGTM!Adding the neutrons channel is consistent with the GitHub Actions workflow update and enables access to packages hosted in the neutrons channel.
102-102: LGTM!The plot-publisher dependency is correctly declared in the run-dependencies section with the same version constraint as in the development dependencies.
87-87: No changes needed. The plot-publisher dependency with version constraint>=1.0,<2.0is correctly specified and actively used in the codebase (imported insrc/lr_autoreduce/reduce_REF_L.py). The version constraint appropriately pins the major version while allowing compatible 1.x releases.src/lr_reduction/template.py (2)
7-7: LGTM!The Path import is necessary for the new template file selection logic and follows standard library conventions.
37-64: LGTM!The template selection logic is well-implemented with clear geometry-based branching, appropriate fallback to a default template, and proper error handling when no template is available.
tests/unit/lr_autoreduce/test_reduce_ref_l.py (7)
10-27: LGTM!The test correctly verifies that minimal required arguments are parsed and all optional arguments receive appropriate default values.
30-52: LGTM!The test thoroughly validates that all positional arguments are correctly parsed and assigned when provided.
55-65: LGTM!The test correctly validates that the --no_publish flag is properly parsed as a boolean flag.
68-72: LGTM!The test adequately covers the str_to_bool utility function with both true and false cases, including case-insensitivity.
75-78: LGTM!The sample_logs fixture provides the minimal structure required for testing the confirm_data_availability function.
81-87: LGTM!The test correctly verifies that CalledProcessError exceptions are caught and logged with an appropriate warning message.
90-105: LGTM!These tests comprehensively verify that all expected subprocess exceptions (FileNotFoundError and TimeoutExpired) are properly caught and logged with appropriate warning messages.
src/lr_reduction/mantid_utils.py (1)
5-39: LGTM!The SampleLogs class provides a clean wrapper around Mantid's run object, simplifying property value access by handling both scalar and vector properties transparently. The implementation is clear and well-documented.
src/lr_autoreduce/reduce_REF_L.py (4)
23-40: LGTM!The imports are well-organized and include all necessary modules for the autoreduction workflow. The CONDA_ENV constant is clearly defined and documented.
42-70: LGTM!The argument parser is well-structured with clear help text and appropriate default values. Maintaining compatibility arguments (old_version_flag, fit_first_peak) is a reasonable design choice.
118-173: LGTM!The upload_report function properly handles both sequence and single-run cases, with graceful error handling for missing or corrupted partial data files. The multiplot logic is clear and the fallback mechanism is appropriate.
194-213: LGTM!The main entry point is well-structured with clear argument parsing and type conversion. The str_to_bool utility is simple and effective for the expected use case.
|
Looks good, please squash when merging |
3 participants
Description of work:
This PR uses the current autoreduction script (
/SNS/REF_L/shared/autoreduce/reduce_REF_L.py) as a starting point and splits it into functions and adds tests. The logic for publishing plots will be rewritten in a separate PR to include more metadata and additional plots, therefore, testing of the plotting logic will be added later as part of that work.Additional changes:
SampleLogsand associated tests from https://github.com/neutrons/MagnetismReflectometerCheck all that apply:
Added integration testsReferences:
(Instructions for testing here)
Run autoreduction for one run on the analysis cluster:
Check list for the reviewer