Conversation
| "centers": [0, 72, 86.4, 115.2, 172.8, 273.6, 288.0, 302.4], | ||
| "widths": [2.46, 3.02, 3.27, 3.27, 5.02, 3.93, 3.93, 2.46], | ||
| "direction": "anti-clockwise", | ||
| "PSC1": { |
There was a problem hiding this comment.
I decided it was probably better to have a flat structure where each entry has a "type".
I think it will make it easier to just add/remove components without having to place them in the correct group.
There was a problem hiding this comment.
Pull Request Overview
This PR implements functionality to load model parameters from JSON files, enabling users to define instruments in a structured JSON format rather than programmatically creating components. The key change is adding a Model.from_json() class method that parses JSON instrument definitions and creates appropriate model components.
Key changes:
- Added
Model.from_json()class method for loading instruments from JSON files - Refactored facility data structures to use the new JSON format with explicit units
- Added comprehensive test coverage for JSON loading functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tof/model.py | Added from_json() class method and helper functions for parsing JSON instrument definitions |
| src/tof/source.py | Converted public attributes to properties with private backing fields |
| tests/model_test.py | Added comprehensive tests for JSON loading with and without sources |
| src/tof/facilities/ess/odin.py | Updated data structure to new JSON format with explicit units |
| src/tof/facilities/ess/dream.py | Updated data structure to new JSON format with explicit units |
| src/tof/facilities/common.py | Removed file as functionality moved to model.py |
| docs/components.ipynb | Added documentation and examples for JSON loading feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@nvaytet I gave this to Claude Code to review, just to see how it works (I did not install the GitHub app, just pointed a local instance to this PR). Here is the report it made. I did not check all details, but most of it seems factual. Is this useful for you? Edit: I you like I can ask it to implement the suggested fixed? PR Review: scipp/tof #98 - Load parameters from JSONReviewer: Claude Code SummaryThis PR adds the ability to load instrument models from JSON files via Changes Overview:
🔴 Critical Issues1. Type Annotation MismatchLocation: src/tof/model.py:102 Issue: The # Current signature (INCORRECT)
def __init__(
self,
source: Source, # ❌ Type says Source required
choppers: Chopper | list[Chopper] | tuple[Chopper, ...] | None = None,
detectors: Detector | list[Detector] | tuple[Detector, ...] | None = None,
):But in source = None # Can be None!
return cls(source=source, **beamline)Impact: Type checkers (mypy, pyright) won't catch potential Fix: def __init__(
self,
source: Source | None, # ✅ Correctly allows None
choppers: Chopper | list[Chopper] | tuple[Chopper, ...] | None = None,
detectors: Detector | list[Detector] | tuple[Detector, ...] | None = None,
):2. Silent Ignoring of Unknown Component TypesLocation: src/tof/model.py:52-82 Issue: The def make_beamline(instrument: dict) -> dict[str, list[Chopper] | list[Detector]]:
choppers = []
detectors = []
for name, comp in instrument.items():
if comp["type"] == "chopper":
choppers.append(...)
elif comp["type"] == "detector":
detectors.append(...)
# ❌ Unknown types are silently ignored!
return {"choppers": choppers, "detectors": detectors}Example: params = {
"monitor": {"type": "monitor", "distance": {"value": 10.0, "unit": "m"}},
# ^^^ Typo or unsupported type - silently ignored, no error!
}Impact: Configuration errors go unnoticed. A user might spend time debugging why their component isn't loaded. Fix: def make_beamline(instrument: dict) -> dict[str, list[Chopper] | list[Detector]]:
choppers = []
detectors = []
for name, comp in instrument.items():
comp_type = comp.get("type")
if comp_type == "chopper":
choppers.append(...)
elif comp_type == "detector":
detectors.append(...)
elif comp_type == "source":
continue # Handled separately in from_json()
else:
raise ValueError(
f"Unknown component type '{comp_type}' for component '{name}'. "
f"Supported types: 'chopper', 'detector', 'source'"
)
return {"choppers": choppers, "detectors": detectors}3. Poor Error Message for Invalid DirectionLocation: src/tof/model.py:61-63 Issue: When an invalid direction like "counterclockwise" is used, the error is a confusing direction={"clockwise": Clockwise, "anti-clockwise": AntiClockwise}[
comp["direction"] # ❌ KeyError: 'counterclockwise'
]Example Error: Impact: User doesn't know what valid values are or where the error occurred. Fix: direction_str = comp["direction"]
if direction_str not in {"clockwise", "anti-clockwise"}:
raise ValueError(
f"Invalid direction '{direction_str}' for chopper '{name}'. "
f"Must be 'clockwise' or 'anti-clockwise'."
)
direction = {"clockwise": Clockwise, "anti-clockwise": AntiClockwise}[direction_str]
|
Yes, very much so.
Yes please! Edit: I did it myself 😎 |
|
Merging as reviewed quite thoroughly by Claude |
This allows to read in model params from a JSON file.
Example: