Side by side projection in new Instrument View #39949
Conversation
jclarkeSTFC
added
the
Epic
By moving as much maths as possible to a separate class, we can then use this class in the new Instrument View window to avoid code duplication. Added some tests as well.
Also added tests for the Python interface.
Added the method to the Python interface, and added C++ and Python tests.
All the information we need is on the GridDetector base class, so may as well use that instead of RectangularDetector.
It's possible to specify a bank's position in the side-by-side view in the IDF. If that's happened, then put the panel in that position.
At least one instrument, WISH, has multiple detector IDs at the same position. We only plot one of those, currently the first, but the side-by-side projection needs to get the relevant component indices for the given detectors. This is the index of the given detectors in the list of all the detectors.
jclarkeSTFC
force-pushed
the
side-by-side-projection
branch
from
9ae0eba to
dea2ccb
Compare
github-actions
bot
added
the
Has Conflicts
|
👋 Hi, @jclarkeSTFC, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
To flatten the detector IDs of the tube banks and the grid banks together, we need to use np.concatenate, because we can't make an array with 2 lists of different lengths.
jclarkeSTFC
removed
the
Has Conflicts
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new public API class PanelsSurfaceCalculator (header, source, C++ tests) and Boost.Python bindings; PanelsSurface is refactored to delegate geometry computations to this calculator. Introduces a SideBySide projection and FlatBankInfo for instrument view with Python tests, and updates FullInstrumentViewModel/Presenter to use projection-option strings. Changes Projection, SphericalProjection, and CylindricalProjection interfaces (parameterless _calculate_2d_coordinates). Instrument gains a template findDetectorsOfType and findGridDetectors; GridDetector Python exporter added. Also: Python/CMake test entries, scalar_bar_args for mesh rendering, a reflectometry enum rename, and a Cppcheck suppression removal. Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Framework/Reflectometry/src/SpecularReflectionPositionCorrect2.cpp (1)
115-122: Bug: wrong validator updated; makes LinePosition > 0 instead of ≥ 0.You set lowerExclusive on nonNegativeDouble instead of positiveDouble, tightening LinePosition unexpectedly.
Apply this diff:
auto nonNegativeDouble = std::make_shared<Kernel::BoundedValidator<double>>(); nonNegativeDouble->setLower(0.); declareProperty("LinePosition", EMPTY_DBL(), nonNegativeDouble, "A fractional workspace index for the specular line centre."); auto positiveDouble = std::make_shared<Kernel::BoundedValidator<double>>(); - nonNegativeDouble->setLowerExclusive(0.); + positiveDouble->setLowerExclusive(0.); declareProperty("PixelSize", EMPTY_DBL(), positiveDouble, "Size of a detector pixel, in metres.");
🧹 Nitpick comments (23)
qt/python/instrumentview/instrumentview/Projections/CylindricalProjection.py (1)
20-26: Guard against division by zero in normalization.When a detector coincides with the sample, norm can be zero.
Apply this diff:
- v_scale = 1.0 / np.sqrt(x * x + y * y + z * z) + norm = np.sqrt(x * x + y * y + z * z) + v_scale = np.divide(1.0, norm, out=np.zeros_like(norm), where=norm > 0)qt/python/instrumentview/instrumentview/Projections/SphericalProjection.py (1)
20-24: Numerical stability: avoid divide-by-zero and acos domain errors.Clamp v/r into [-1,1] and handle r==0.
Apply this diff:
- r = np.sqrt(x * x + y * y + v * v) - - u = -np.atan2(y, x) - v = -np.acos(v / r) + r = np.sqrt(x * x + y * y + v * v) + + u = -np.atan2(y, x) + vr = np.divide(v, r, out=np.zeros_like(v), where=r > 0) + v = -np.arccos(np.clip(vr, -1.0, 1.0))Framework/Geometry/inc/MantidGeometry/Instrument.h (1)
231-254: Be explicit about GridDetector completeness in header to avoid template instantiation pitfalls.dynamic_pointer_cast to T in a header-defined template can require T to be complete at instantiation sites. Adding GridDetector include here avoids ODR/compile surprises.
Apply this diff near the includes:
#include "MantidGeometry/Instrument/ObjComponent.h" #include "MantidGeometry/Instrument/RectangularDetector.h" +#include "MantidGeometry/Instrument/GridDetector.h" #include "MantidGeometry/Instrument_fwd.h"qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py (3)
19-36: Annotate class-level constants as ClassVar to satisfy linters (RUF012).Prevents treating them as instance attributes.
Apply this diff (and add the import):
-from instrumentview.Projections.SideBySide as iv_side_by_side +import instrumentview.Projections.SideBySide as iv_side_by_side +from typing import ClassVar @@ - _FULL_3D = "3D" - _SPHERICAL_X = "Spherical X" - _SPHERICAL_Y = "Spherical Y" - _SPHERICAL_Z = "Spherical Z" - _CYLINDRICAL_X = "Cylindrical X" - _CYLINDRICAL_Y = "Cylindrical Y" - _CYLINDRICAL_Z = "Cylindrical Z" - _SIDE_BY_SIDE = "Side by Side" - _PROJECTION_OPTIONS = [ + _FULL_3D: ClassVar[str] = "3D" + _SPHERICAL_X: ClassVar[str] = "Spherical X" + _SPHERICAL_Y: ClassVar[str] = "Spherical Y" + _SPHERICAL_Z: ClassVar[str] = "Spherical Z" + _CYLINDRICAL_X: ClassVar[str] = "Cylindrical X" + _CYLINDRICAL_Y: ClassVar[str] = "Cylindrical Y" + _CYLINDRICAL_Z: ClassVar[str] = "Cylindrical Z" + _SIDE_BY_SIDE: ClassVar[str] = "Side by Side" + _PROJECTION_OPTIONS: ClassVar[list[str]] = [
76-77: Initialize projection array with explicit shape/dtype.zeros_like can inherit object dtype if detector_positions is object; prefer float array.
Apply this diff:
- self._detector_projection_positions = np.zeros_like(self.detector_positions) + self._detector_projection_positions = np.zeros((len(self.detector_positions), 3), dtype=float)
212-229: Dispatch logic looks good; verify Projection base expects numeric Nx3 positions.Cylindrical/Spherical now compute numpy dots; ensure detector_positions are float Nx3, not V3D objects.
- If not guaranteed by Projection.init, convert in setup() when building self._detector_positions:
- self._detector_positions = np.array(detector_positions) + self._detector_positions = np.array([[p.X(), p.Y(), p.Z()] for p in detector_positions], dtype=float)
- Minor nit (TRY003): keep the ValueError short, e.g., ValueError(f"Unknown projection: {projection_option}").
qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py (1)
363-366: Scalar bar args pass‑through — LGTM; consider making show/hide explicitBe explicit to avoid backend/version defaults changing behavior.
- scalar_bar_args = dict(interactive=True, vertical=True, title_font_size=15, label_font_size=12) if scalars is not None else None - self.main_plotter.add_mesh( - mesh, pickable=False, scalars=scalars, render_points_as_spheres=True, point_size=15, scalar_bar_args=scalar_bar_args - ) + scalar_bar_args = dict(interactive=True, vertical=True, title_font_size=15, label_font_size=12) if scalars is not None else None + self.main_plotter.add_mesh( + mesh, + pickable=False, + scalars=scalars, + render_points_as_spheres=True, + point_size=15, + show_scalar_bar=scalars is not None, + scalar_bar_args=scalar_bar_args, + )qt/widgets/instrumentview/test/InstrumentWidget/PanelsSurfaceTest.h (1)
35-36: Avoid shadowing base member name for calculator in test helperPanelsSurface already has a private m_calculator. Shadowing can confuse readers; prefer a distinct name.
- Mantid::Kernel::Quat calcBankRotation(const Mantid::Kernel::V3D &detPos, Mantid::Kernel::V3D normal) { - return this->m_calculator.calcBankRotation(detPos, normal, m_zaxis, m_yaxis, m_pos); + Mantid::Kernel::Quat calcBankRotation(const Mantid::Kernel::V3D &detPos, Mantid::Kernel::V3D normal) { + return this->m_testCalculator.calcBankRotation(detPos, normal, m_zaxis, m_yaxis, m_pos); } @@ -private: - PanelsSurfaceCalculator m_calculator; +private: + PanelsSurfaceCalculator m_testCalculator;Also applies to: 46-48
qt/python/instrumentview/instrumentview/test/test_model.py (1)
129-129: Projection option constants in tests — LGTM; add side‑by‑side coverageConsider adding a test exercising the new side‑by‑side projection option to lock in dispatch and output shape.
Also applies to: 133-133, 135-144
qt/python/instrumentview/instrumentview/Projections/Projection.py (2)
26-31: Normalize input types to np.ndarray float64 for robustness.Currently sample_position/root_position are not converted, which can break root_position.dot(...). Convert both and prefer np.asarray for consistency with detector_positions/axis.
Apply this diff:
- self._sample_position = sample_position - self._root_position = root_position - self._detector_positions = np.array(detector_positions) - self._projection_axis = np.array(axis, dtype=np.float64) + self._sample_position = np.asarray(sample_position, dtype=np.float64) + self._root_position = np.asarray(root_position, dtype=np.float64) + self._detector_positions = np.asarray(detector_positions, dtype=np.float64) + self._projection_axis = np.asarray(axis, dtype=np.float64)
42-46: Use np.dot to avoid attribute errors with non-ndarray inputs.Calling list.dot(...) would fail if root_position isn’t a NumPy array.
Apply this diff:
- z = root_position.dot(self._projection_axis) + z = float(np.dot(root_position, self._projection_axis))qt/python/instrumentview/instrumentview/Projections/test/test_SideBySide.py (3)
22-32: Avoid mutable default arguments.Use None default and initialize inside to prevent shared-state bugs across tests.
Apply this diff:
- def _create_flat_bank_info(self, use_relative_positions: bool, detector_ids: list[int] = [5, 10, 15]) -> FlatBankInfo: + def _create_flat_bank_info(self, use_relative_positions: bool, detector_ids: list[int] | None = None) -> FlatBankInfo: flat_bank = FlatBankInfo() flat_bank.reference_position = np.zeros(3) - flat_bank.detector_ids = detector_ids + flat_bank.detector_ids = detector_ids if detector_ids is not None else [5, 10, 15]
72-73: Minor style: prefer iterable unpacking over concatenation.Slightly clearer and avoids creating an intermediate list.
Apply this diff:
- mock_detector_info.detectorIDs.return_value = detector_ids + [100] if has_other_detectors else detector_ids + mock_detector_info.detectorIDs.return_value = ([*detector_ids, 100] if has_other_detectors else detector_ids)
101-101: Silence unused patched-arg warnings by prefixing with underscore.Rename unused mock parameters (e.g., mock_panels_surface_calculator) to _ to appease linters.
Also applies to: 113-113, 117-117, 133-133, 149-149, 167-167, 179-179, 192-192, 205-205
Framework/PythonInterface/mantid/api/src/Exports/PanelsSurfaceCalculator.cpp (3)
60-62: Make calculatePanelNormal return explicit [x, y, z] for consistency.Align with calculateBankNormal suggested fix to avoid relying on implicit conversions.
Apply this diff:
- const auto panelNormal = self.calculatePanelNormal(panelCornersVec); - list panelNormalList(panelNormal); - return panelNormalList; + const auto panelNormal = self.calculatePanelNormal(panelCornersVec); + list panelNormalList; + panelNormalList.append(panelNormal.X()); + panelNormalList.append(panelNormal.Y()); + panelNormalList.append(panelNormal.Z()); + return panelNormalList;
216-217: Typo in docstring: “quarternion” → “quaternion”.Apply this diff:
- "provided as a list containing the real and imaginary parts of a quarternion (length 4).") + "provided as a list containing the real and imaginary parts of a quaternion (length 4).")
7-7: Nit: #pragma once in a .cpp is unnecessary.Safe to remove; no behavior impact.
Framework/API/inc/MantidAPI/PanelsSurfaceCalculator.h (2)
48-50: Avoid copying shared_ptr parameterPass Instrument_const_sptr by const reference to avoid unnecessary shared_ptr copies.
- std::optional<Kernel::V2D> getSideBySideViewPos(const ComponentInfo &componentInfo, - const Instrument_const_sptr instrument, + std::optional<Kernel::V2D> getSideBySideViewPos(const ComponentInfo &componentInfo, + const Instrument_const_sptr &instrument, const size_t componentIndex) const;
53-54: Consider a static loggerLogger can be static to avoid per-instance copies. Not blocking.
-private: - Mantid::Kernel::Logger g_log = Mantid::Kernel::Logger("PanelsSurfaceCalculator"); +private: + static Mantid::Kernel::Logger g_log;Add a definition in the .cpp:
Mantid::Kernel::Logger PanelsSurfaceCalculator::g_log("PanelsSurfaceCalculator");qt/python/instrumentview/instrumentview/Projections/SideBySide.py (3)
100-109: Ensure integer dtype for concatenation and None-check for optional flat bank
- detectors_in_banks should be int dtype to avoid ID type drift.
- remaining_detectors_bank should be checked against None explicitly.
- detectors_in_banks = np.empty(0) if len(self._flat_banks) == 0 else np.concatenate([d.detector_ids for d in self._flat_banks]) - remaining_detectors_bank = self._create_flat_bank_with_missing_detectors(detectors_in_banks) - if remaining_detectors_bank: + detectors_in_banks = (np.empty(0, dtype=int) if len(self._flat_banks) == 0 + else np.concatenate([np.array(d.detector_ids, dtype=int) for d in self._flat_banks])) + remaining_detectors_bank = self._create_flat_bank_with_missing_detectors(detectors_in_banks) + if remaining_detectors_bank is not None: self._flat_banks.append(remaining_detectors_bank)
197-206: Return Optional and keep integer dtype for detectors
- Function returns None in one branch; annotate Optional.
- Keep IDs as ints.
- def _create_flat_bank_with_missing_detectors(self, detectors_already_in_banks: np.ndarray) -> FlatBankInfo: - detectors = np.array(self._detector_ids) + def _create_flat_bank_with_missing_detectors(self, detectors_already_in_banks: np.ndarray) -> Optional[FlatBankInfo]: + detectors = np.array(self._detector_ids, dtype=int)
93-93: Silence unused-argument warning for root_positionThis arg is unused by design; rename to _root_position.
- def _calculate_axes(self, root_position: np.ndarray) -> None: + def _calculate_axes(self, _root_position: np.ndarray) -> None:Framework/API/src/PanelsSurfaceCalculator.cpp (1)
137-138: Static logger access if made static; otherwise OKIf you adopt the static logger suggestion, drop this->.
- this->g_log.warning() << "Assembly " << componentInfo.name(bankIndex) << " isn't flat.\n"; + g_log.warning() << "Assembly " << componentInfo.name(bankIndex) << " isn't flat.\n"; @@ - this->g_log.warning() << "Colinear Assembly.\n"; + g_log.warning() << "Colinear Assembly.\n"; @@ - this->setBankVisited(componentInfo, bankIndex, visited); + setBankVisited(componentInfo, bankIndex, visited);Also applies to: 174-175, 357-359
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
Framework/API/CMakeLists.txt(3 hunks)Framework/API/inc/MantidAPI/PanelsSurfaceCalculator.h(1 hunks)Framework/API/src/PanelsSurfaceCalculator.cpp(1 hunks)Framework/API/test/PanelsSurfaceCalculatorTest.h(1 hunks)Framework/Geometry/inc/MantidGeometry/Instrument.h(3 hunks)Framework/Geometry/src/Instrument.cpp(0 hunks)Framework/PythonInterface/mantid/api/CMakeLists.txt(1 hunks)Framework/PythonInterface/mantid/api/src/Exports/PanelsSurfaceCalculator.cpp(1 hunks)Framework/PythonInterface/mantid/geometry/src/Exports/GridDetector.cpp(2 hunks)Framework/PythonInterface/mantid/geometry/src/Exports/Instrument.cpp(1 hunks)Framework/PythonInterface/test/python/mantid/api/CMakeLists.txt(1 hunks)Framework/PythonInterface/test/python/mantid/api/PanelsSurfaceCalculatorTest.py(1 hunks)Framework/Reflectometry/src/SpecularReflectionPositionCorrect2.cpp(4 hunks)buildconfig/CMake/CppCheck_Suppressions.txt.in(0 hunks)qt/python/instrumentview/CMakeLists.txt(1 hunks)qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py(6 hunks)qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py(3 hunks)qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py(1 hunks)qt/python/instrumentview/instrumentview/Projections/CylindricalProjection.py(1 hunks)qt/python/instrumentview/instrumentview/Projections/Projection.py(2 hunks)qt/python/instrumentview/instrumentview/Projections/SideBySide.py(1 hunks)qt/python/instrumentview/instrumentview/Projections/SphericalProjection.py(1 hunks)qt/python/instrumentview/instrumentview/Projections/test/test_SideBySide.py(1 hunks)qt/python/instrumentview/instrumentview/test/test_model.py(1 hunks)qt/python/instrumentview/instrumentview/test/test_view.py(1 hunks)qt/widgets/instrumentview/inc/MantidQtWidgets/InstrumentView/PanelsSurface.h(2 hunks)qt/widgets/instrumentview/src/PanelsSurface.cpp(12 hunks)qt/widgets/instrumentview/test/InstrumentWidget/PanelsSurfaceTest.h(2 hunks)
💤 Files with no reviewable changes (2)
- Framework/Geometry/src/Instrument.cpp
- buildconfig/CMake/CppCheck_Suppressions.txt.in
🧰 Additional context used
🪛 Ruff (0.13.1)
qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py
124-124: Avoid specifying long messages outside the exception class
(TRY003)
qt/python/instrumentview/instrumentview/Projections/test/test_SideBySide.py
22-22: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
72-72: Consider [*detector_ids, 100] instead of concatenation
Replace with [*detector_ids, 100]
(RUF005)
101-101: Unused method argument: mock_panels_surface_calculator
(ARG002)
113-113: Unused method argument: mock_panels_surface_calculator
(ARG002)
117-117: Unused method argument: mock_calculate_coordinates
(ARG002)
133-133: Unused method argument: mock_panels_surface_calculator
(ARG002)
149-149: Unused method argument: mock_panels_surface_calculator
(ARG002)
149-149: Unused method argument: mock_calculate_detector_coords
(ARG002)
167-167: Unused method argument: mock_panels_surface_calculator
(ARG002)
179-179: Unused method argument: mock_panels_surface_calculator
(ARG002)
192-192: Unused method argument: mock_panels_surface_calculator
(ARG002)
192-192: Unused method argument: mock_calculate_detector_coords
(ARG002)
205-205: Unused method argument: mock_panels_surface_calculator
(ARG002)
205-205: Unused method argument: mock_calculate_detector_coords
(ARG002)
qt/python/instrumentview/instrumentview/Projections/SideBySide.py
22-22: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
61-61: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
62-62: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
93-93: Unused method argument: root_position
(ARG002)
263-263: Avoid specifying long messages outside the exception class
(TRY003)
qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py
27-36: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
227-227: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (30)
Framework/Reflectometry/src/SpecularReflectionPositionCorrect2.cpp (1)
21-21: Enum rename is fine.Rename to RotationPlane is clear and scoped. No issues.
qt/python/instrumentview/instrumentview/Projections/CylindricalProjection.py (1)
14-16: Signature refactor to use internal state looks good.Matches the new Projection interface and simplifies calls.
Please confirm Projection.positions() now calls _calculate_2d_coordinates() without args everywhere.
Framework/PythonInterface/mantid/geometry/src/Exports/GridDetector.cpp (1)
11-16: Python export of GridDetector_const_sptr and vector wrapper looks correct.Good addition; aligns with new grid-detector APIs.
- Ensure std_vector_grid_detector naming matches existing conventions for other vector exporters.
- Confirm at least one Python test imports and uses the new export (e.g., from mantid.geometry import std_vector_grid_detector).
Also applies to: 27-30
qt/python/instrumentview/CMakeLists.txt (1)
15-16: Add SideBySide tests to suite — good.Direct inclusion avoids filename-pattern issues.
qt/python/instrumentview/instrumentview/Projections/SphericalProjection.py (1)
14-16: Signature refactor aligns with stateful Projection.Consistent with CylindricalProjection refactor.
Framework/Geometry/inc/MantidGeometry/Instrument.h (2)
208-212: Public helpers for detector discovery are a nice addition.findRectDetectors delegating to a generic finder and new findGridDetectors improve reuse.
271-276: Default member initializers for caches are fine.Clearer than relying on constructors.
qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py (3)
59-60: Nice: disable logging for CreateDetectorTable.Reduces noise in tests/UI.
119-121: Confirm consumers expect only valid-detector positions.Property now returns the full internal array (already sized to valid detectors). Ensure the view/presenter doesn’t expect masking at access time.
212-229: Projection API: parameterless _calculate_2d_coordinates confirmed — original assumption incorrectpositions() returns stored x/y arrays; the base Projection calls self._calculate_2d_coordinates() with no arguments (stores _detector_x/y), and subclasses implement def _calculate_2d_coordinates(self) in Projection.py, SideBySide.py, CylindricalProjection.py, SphericalProjection.py.
Likely an incorrect or invalid review comment.
Framework/PythonInterface/test/python/mantid/api/CMakeLists.txt (1)
39-39: Add PanelsSurfaceCalculator Python test — LGTMIncluded in TEST_PY_FILES and validated via check_tests_valid.
Framework/PythonInterface/mantid/api/CMakeLists.txt (1)
93-93: Bindings wiring for PanelsSurfaceCalculator — LGTMOrder-sensitive list respected; no dependency inversions apparent.
qt/widgets/instrumentview/inc/MantidQtWidgets/InstrumentView/PanelsSurface.h (1)
9-9: PanelsSurfaceCalculator integration — LGTMIncluding the calculator in the header and storing by value is appropriate given usage.
Also applies to: 116-118
Framework/API/CMakeLists.txt (1)
123-123: API, header, and tests wired for PanelsSurfaceCalculator — LGTMBuild lists updated consistently; test added to TEST_FILES.
Also applies to: 322-322, 457-457
Framework/PythonInterface/mantid/geometry/src/Exports/Instrument.cpp (1)
88-90: OK — GridDetector Python pointer + vector exporter registeredregister_ptr_to_python<GridDetector_const_sptr>() and std_vector_exporter<GridDetector_const_sptr, /NoProxy=/true>::wrap("std_vector_grid_detector") are present in Framework/PythonInterface/mantid/geometry/src/Exports/GridDetector.cpp — no action required.
qt/python/instrumentview/instrumentview/test/test_view.py (1)
66-72: LGTM: scalar_bar_args assertion aligns with new API.The added scalar_bar_args in the expectation looks correct and matches the intended PyVista usage.
qt/python/instrumentview/instrumentview/Projections/Projection.py (3)
48-53: LGTM: canonical axis selection is now explicit float64.
66-67: LGTM: internal call updated to parameterless method.
60-61: Abstract method signature change — subclasses updated. Verified SphericalProjection.py, CylindricalProjection.py and SideBySide.py define def _calculate_2d_coordinates(self) -> tuple[np.ndarray, np.ndarray] (no extra parameters).Framework/PythonInterface/test/python/mantid/api/PanelsSurfaceCalculatorTest.py (1)
24-31: LGTM: setupBasisAxes round-trips x/y via binding mutability.Framework/API/test/PanelsSurfaceCalculatorTest.h (2)
24-39: LGTM: basis validation is thorough (orthogonality and normalization).
110-121: LGTM: transformedBoundingBoxPoints assertions align with C++ angle-axis API.qt/widgets/instrumentview/src/PanelsSurface.cpp (5)
185-201: LGTM: corners/normal/rotation refactor is clean and cohesive.
269-282: LGTM: bank-flatness detection moved to calculator (clearer responsibility).
409-414: LGTM: seeding polygon with transformed bounding-box points improves initial layout.
441-458: LGTM: visited-state updates via calculator centralize traversal logic.
175-178: LGTM: basis axes now delegated to calculator. m_calculator is a value member (Mantid::API::PanelsSurfaceCalculator) declared in qt/widgets/instrumentview/inc/MantidQtWidgets/InstrumentView/PanelsSurface.h (line 117) and is default-constructed — PanelsSurfaceCalculator has a no-arg constructor (used in tests), so no further action required.qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py (2)
105-127: calculate_projection signature matches callsite — no action required.
Signature found: calculate_projection(self, projection_option: str, axis: list[int]) in qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py; callsite model.calculate_projection(projection_type, axis) is correct.
70-75: Model provides _PROJECTION_OPTIONS and default_projection; Presenter fallback handles missing defaultFullInstrumentViewModel defines _PROJECTION_OPTIONS and a default_projection property; projection_combo_options already catches ValueError and falls back to index 0, so no change required.
qt/python/instrumentview/instrumentview/Projections/SideBySide.py (1)
148-186: Incorrect — getAllTubeDetectorFlatGroupParents is already exported
Bindings are implemented in Framework/PythonInterface/mantid/api/src/Exports/PanelsSurfaceCalculator.cpp (exports getAllTubeDetectorFlatGroupParents) and referenced in qt/python/instrumentview/instrumentview/Projections/test/test_SideBySide.py.Likely an incorrect or invalid review comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qt/python/instrumentview/instrumentview/Projections/Projection.py (1)
38-39: Use normalized/root-cached vector when computing axes.You coerce root_position to float64 in self._root_position but call _calculate_axes with the unnormalized argument.
Apply this diff:
- self._calculate_axes(root_position) + self._calculate_axes(self._root_position)
🧹 Nitpick comments (18)
Framework/Geometry/inc/MantidGeometry/Instrument.h (1)
12-12: Avoid heavy include in public header; prefer forward declaration or fwd headerIncluding GridDetector.h here increases compile-time and coupling. If GridDetector_const_sptr is available via Instrument_fwd.h (already included), drop this include and rely on forward declarations.
Framework/API/inc/MantidAPI/PanelsSurfaceCalculator.h (1)
9-20: Ensure size_t is declared in this public headerAdd for size_t.
#include "MantidKernel/V3D.h" +#include <cstddef> #include <functional> #include <optional> #include <vector>Framework/API/src/PanelsSurfaceCalculator.cpp (2)
227-264: Use tolerance for colinearity/opposition checks, not exact equalityFloating-point exact equality on vectors is brittle. Prefer dot-product with Mantid::Kernel::Tolerance.
- if (directionToViewer == -normal) { + if (std::abs(directionToViewer.scalar_prod(normal)) > 1.0 - Mantid::Kernel::Tolerance && + directionToViewer.scalar_prod(normal) < 0) { return Mantid::Kernel::Quat(0, yAxis.X(), yAxis.Y(), yAxis.Z()); // 180 degree rotation about y axis - } else if (normal.cross_prod(directionToViewer).nullVector()) { + } else if (normal.cross_prod(directionToViewer).norm() <= Mantid::Kernel::Tolerance) { return Mantid::Kernel::Quat(); }
32-46: Normalize x-axis after recomputing itxaxis is recomputed but left unnormalized; normalize for an orthonormal basis.
yaxis = zaxis.cross_prod(xaxis); yaxis.normalize(); - xaxis = yaxis.cross_prod(zaxis); + xaxis = yaxis.cross_prod(zaxis); + xaxis.normalize();qt/python/instrumentview/instrumentview/Projections/Projection.py (1)
24-31: Strong typing and coercion are good; add minimal shape validation to prevent silent object arrays.Guard against non-(N,3) inputs and object dtypes early.
Apply this diff:
self._sample_position = np.asarray(sample_position, dtype=np.float64) self._root_position = np.asarray(root_position, dtype=np.float64) self._detector_positions = np.asarray(detector_positions, dtype=np.float64) self._projection_axis = np.asarray(axis, dtype=np.float64) self._x_axis = np.zeros_like(self._projection_axis, dtype=np.float64) self._y_axis = np.zeros_like(self._projection_axis, dtype=np.float64) + # Basic shape validation + if self._detector_positions.ndim != 2 or self._detector_positions.shape[1] != 3: + raise ValueError("detector_positions must be an (N, 3) float array") + if self._sample_position.shape != (3,) or self._root_position.shape != (3,) or self._projection_axis.shape != (3,): + raise ValueError("sample_position, root_position, and axis must be length-3 float vectors")qt/python/instrumentview/instrumentview/Projections/test/test_SideBySide.py (5)
72-73: Prefer literal-unpack over concatenation.Improves readability and satisfies RUF005.
Apply this diff:
- mock_detector_info.detectorIDs.return_value = detector_ids + [100] if has_other_detectors else detector_ids + mock_detector_info.detectorIDs.return_value = ([*detector_ids, 100] if has_other_detectors else detector_ids)
100-104: Silence unused patched arg to satisfy linters.The injected mock object isn’t referenced.
Apply this diff:
- def test_calculate_axes(self, mock_panels_surface_calculator): + def test_calculate_axes(self, _mock_panels_surface_calculator):
111-119: Rename unused patched args with underscore.Avoids ARG002 warnings without changing behavior.
Apply this diff:
- self, - mock_panels_surface_calculator, - mock_construct_rectangles, - mock_construct_tube_banks, - mock_create_flat_bank, - mock_calculate_coordinates, - mock_arrange_panels, + self, + _mock_panels_surface_calculator, + mock_construct_rectangles, + mock_construct_tube_banks, + mock_create_flat_bank, + _mock_calculate_coordinates, + mock_arrange_panels,
147-165: Rename unused patched args with underscore.Cleans up multiple ARG002 lints.
Apply this diff:
- def test_construct_tube_banks(self, mock_panels_surface_calculator, mock_calculate_detector_coords): + def test_construct_tube_banks(self, _mock_panels_surface_calculator, _mock_calculate_detector_coords):
190-202: Be explicit about dict ordering in assertions.If detector_id_position_map ordering isn’t guaranteed, compare after sorting by key to avoid flakiness.
Apply this diff:
- np.testing.assert_allclose( - [[0, 0, 0], [0, separation, 0], [separation, 0, 0], [separation, separation, 0]], flat_bank.relative_projected_positions - ) + expected = [[0, 0, 0], [0, separation, 0], [separation, 0, 0], [separation, separation, 0]] + items = sorted(flat_bank.detector_id_position_map.items()) + _, positions = zip(*items) + np.testing.assert_allclose(expected, positions)Framework/PythonInterface/test/python/mantid/api/PanelsSurfaceCalculatorTest.py (1)
66-75: Minor naming nit: use snake_case consistently.No behavior change; improves readability.
Apply this diff:
- detector_Position = [1, 0, 0] + detector_position = [1, 0, 0] @@ - rotation = self._calculator.calcBankRotation(detector_Position, normal, z_axis, y_axis, sample_position) + rotation = self._calculator.calcBankRotation(detector_position, normal, z_axis, y_axis, sample_position)Framework/PythonInterface/mantid/api/src/Exports/PanelsSurfaceCalculator.cpp (2)
37-46: Return corners as numeric triples for robustness.Explicitly materialize [x,y,z] lists to avoid relying on implicit V3D-to-sequence conversion.
Apply this diff:
- for (size_t i = 0; i < panelCorners.size(); i++) { - list corner(panelCorners[i]); - panelCornersList.append(corner); - } + for (const auto &c : panelCorners) { + list corner; + corner.append(c.X()); + corner.append(c.Y()); + corner.append(c.Z()); + panelCornersList.append(corner); + }
206-208: Add self argument in Python signature for consistency.Improves help() signatures and parity with other methods.
Apply this diff:
- .def("findNumDetectors", &findNumDetectors, (arg("componentInfo"), arg("components")), + .def("findNumDetectors", &findNumDetectors, (arg("self"), arg("componentInfo"), arg("components")), "Finds the number of detectors in a component.")qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py (2)
77-78: Shape choice is sensible; consider 2-column array to reflect 2D output.Not required, but an (N,2) buffer avoids unused Z column.
Apply this diff:
- self._detector_projection_positions = np.zeros_like(self.detector_positions) + self._detector_projection_positions = np.zeros((len(self.detector_positions), 2), dtype=float)And adjust the return to reflect the 2D shape if all consumers accept it.
213-229: Projection dispatch looks good; tighten error message to satisfy Ruff TRY003.Shorter message, same signal.
Apply this diff:
- else: - raise ValueError(f"Unknown projection type: {projection_option}") + else: + raise ValueError("Unknown projection option")qt/python/instrumentview/instrumentview/Projections/SideBySide.py (3)
45-49: Guard against length mismatch between positions and detector_ids.Avoid silent mis-mapping when relative_projected_positions length ≠ detector_ids length.
Apply this diff:
- if len(self.relative_projected_positions) > 0: + if len(self.relative_projected_positions) > 0: + if len(self.detector_ids) != len(self.relative_projected_positions): + raise ValueError("relative_projected_positions must align 1:1 with detector_ids") self._transform_positions_so_origin_bottom_left() for index in range(len(self.detector_ids)): self.detector_id_position_map[self.detector_ids[index]] = self.relative_projected_positions[index] + self.reference_position return
32-36: Avoid shadowing built-in id; use det_id for clarity.Rename loop variables from id to det_id (and update uses) to prevent confusion with built-in id().
Example diffs:
- for id in self.detector_id_position_map: - self.detector_id_position_map[id] += shift + for det_id in self.detector_id_position_map: + self.detector_id_position_map[det_id] += shift- id = self.detector_ids[iy * x_pixels + ix] - detector_position = np.add(origin, [x_offset, y_offset, 0]) - self.detector_id_position_map[id] = detector_position + det_id = self.detector_ids[iy * x_pixels + ix] + detector_position = np.add(origin, [x_offset, y_offset, 0]) + self.detector_id_position_map[det_id] = detector_position- for id in bank.detector_ids: - self._detector_id_to_flat_bank_map[id] = bank + for det_id in bank.detector_ids: + self._detector_id_to_flat_bank_map[det_id] = bank- for id in self._detector_ids: - if id not in self._detector_id_to_flat_bank_map: - raise RuntimeError(f"Detector with ID {id} not found in projection") - bank = self._detector_id_to_flat_bank_map[id] - position = bank.detector_id_position_map[id] + for det_id in self._detector_ids: + if det_id not in self._detector_id_to_flat_bank_map: + raise RuntimeError(f"Detector with ID {det_id} not found in projection") + bank = self._detector_id_to_flat_bank_map[det_id] + position = bank.detector_id_position_map[det_id]Also applies to: 58-60, 116-120, 262-270
258-270: Optional: Raise a more specific exception type.Consider ValueError or KeyError for missing detector IDs to aid callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Framework/API/inc/MantidAPI/PanelsSurfaceCalculator.h(1 hunks)Framework/API/src/PanelsSurfaceCalculator.cpp(1 hunks)Framework/Geometry/inc/MantidGeometry/Instrument.h(4 hunks)Framework/PythonInterface/mantid/api/src/Exports/PanelsSurfaceCalculator.cpp(1 hunks)Framework/PythonInterface/test/python/mantid/api/PanelsSurfaceCalculatorTest.py(1 hunks)qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py(5 hunks)qt/python/instrumentview/instrumentview/Projections/CylindricalProjection.py(1 hunks)qt/python/instrumentview/instrumentview/Projections/Projection.py(2 hunks)qt/python/instrumentview/instrumentview/Projections/SideBySide.py(1 hunks)qt/python/instrumentview/instrumentview/Projections/SphericalProjection.py(1 hunks)qt/python/instrumentview/instrumentview/Projections/test/test_SideBySide.py(1 hunks)qt/python/instrumentview/instrumentview/test/test_model.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qt/python/instrumentview/instrumentview/Projections/CylindricalProjection.py
🧰 Additional context used
🪛 Ruff (0.13.1)
qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py
228-228: Avoid specifying long messages outside the exception class
(TRY003)
qt/python/instrumentview/instrumentview/Projections/SideBySide.py
91-91: Unused method argument: root_position
(ARG002)
265-265: Avoid specifying long messages outside the exception class
(TRY003)
qt/python/instrumentview/instrumentview/Projections/test/test_SideBySide.py
72-72: Consider [*detector_ids, 100] instead of concatenation
Replace with [*detector_ids, 100]
(RUF005)
101-101: Unused method argument: mock_panels_surface_calculator
(ARG002)
113-113: Unused method argument: mock_panels_surface_calculator
(ARG002)
117-117: Unused method argument: mock_calculate_coordinates
(ARG002)
133-133: Unused method argument: mock_panels_surface_calculator
(ARG002)
149-149: Unused method argument: mock_panels_surface_calculator
(ARG002)
149-149: Unused method argument: mock_calculate_detector_coords
(ARG002)
167-167: Unused method argument: mock_panels_surface_calculator
(ARG002)
179-179: Unused method argument: mock_panels_surface_calculator
(ARG002)
192-192: Unused method argument: mock_panels_surface_calculator
(ARG002)
192-192: Unused method argument: mock_calculate_detector_coords
(ARG002)
205-205: Unused method argument: mock_panels_surface_calculator
(ARG002)
205-205: Unused method argument: mock_calculate_detector_coords
(ARG002)
🔇 Additional comments (30)
Framework/Geometry/inc/MantidGeometry/Instrument.h (3)
209-213: New detector finders look goodInline findRectDetectors/findGridDetectors delegating to a generic finder is clean and reusable.
232-255: Generic BFS detector finder looks correctTraversal with validateComponentProperties and assembly-children expansion is sound.
Confirm that validateComponentProperties can’t filter out assemblies that contain target detector types; otherwise a subtree could be skipped inadvertently.
272-276: Safe default initializationDefault-initializing m_sourceCache/m_sampleCache to nullptr is a good safety improvement.
Framework/API/src/PanelsSurfaceCalculator.cpp (9)
15-16: Resolved: required standard headers are now includedand are present. Thanks for addressing.
300-317: Confirm traversal bounds for examineAllComponentsLoop starts at root()-1 and excludes index 0; validate this matches ComponentInfo indexing semantics.
61-66: Guard against empty children when descending to a detectorchildren[0] can be out-of-bounds; stop if no children.
- do { - const auto &children = componentInfo.children(child); - child = children[0]; - } while (!componentInfo.isDetector(child)); + while (!componentInfo.isDetector(child)) { + const auto children = componentInfo.children(child); + if (children.empty()) + break; + child = children.front(); + }
74-81: Fix potential underflow and require ≥2 points for offsetspoints.size() - 1 underflows for size 0; also need a neighbor to diff.
- for (size_t i = 0; i < points.size() - 1; ++i) { + if (points.size() >= 2) { + for (size_t i = 0; i + 1 < points.size(); ++i) { double xdiff = std::abs(points[i + 1].X() - points[i].X()); double ydiff = std::abs(points[i + 1].Y() - points[i].Y()); if (xdiff != 0) xoff = xdiff * 0.5; if (ydiff != 0) yoff = ydiff * 0.5; - } + } + }
120-123: Use V3D::normalize(); no free normalize() existsThis won’t compile as-is.
- const auto normal = normalize(xaxis.cross_prod(yaxis)); - return normal; + auto normal = xaxis.cross_prod(yaxis); + normal.normalize(); + return normal;
135-146: Avoid dangling refs from children(); normalize via member; guard sizechildren() likely returns by value; binding to const& risks UB; also ensure ≥2 children.
- for (auto tube : tubes) { - const auto &children = componentInfo.children(tube); - const auto vector = normalize(componentInfo.position(children[0]) - componentInfo.position(children[1])); - if (std::abs(vector.scalar_prod(normal)) > Mantid::Kernel::Tolerance) { + for (auto tube : tubes) { + const auto children = componentInfo.children(tube); + if (children.size() < 2) + continue; + auto vec = componentInfo.position(children[0]) - componentInfo.position(children[1]); + vec.normalize(); + if (std::abs(vec.scalar_prod(normal)) > Mantid::Kernel::Tolerance) { this->g_log.warning() << "Assembly " << componentInfo.name(bankIndex) << " isn't flat.\n"; return false; } }
354-359: Avoid binding reference to a temporary vector from children()Store by value.
- const auto &bankChildren = componentInfo.children(bankIndex); + const auto bankChildren = componentInfo.children(bankIndex); // Go down the tree to find all the tubes. for (const auto index : bankChildren) addTubes(index, tubes);
189-200: Avoid dangling refs from children() and recurse safelyBind returned vectors by value to avoid lifetime issues.
- const auto &children = componentInfo.children(bankIndex); + const auto children = componentInfo.children(bankIndex); visitedComponents[bankIndex] = true; for (auto child : children) { - const auto &subChildren = componentInfo.children(child); - if (subChildren.size() > 0) + const auto subChildren = componentInfo.children(child); + if (!subChildren.empty()) setBankVisited(componentInfo, child, visitedComponents); else visitedComponents[child] = true; }
155-176: Avoid dangling refs from children(); add minimal safety checksStore children by value; verify indices exist before [0]/[1] usage.
- const auto &tube0 = componentInfo.children(tubes[0]); - const auto &tube1 = componentInfo.children(tubes[1]); + const auto tube0 = componentInfo.children(tubes[0]); + const auto tube1 = componentInfo.children(tubes[1]); + if (tube0.size() < 2 || tube1.size() < 1) { + this->g_log.warning() << "Insufficient tube children to compute normal.\n"; + return Mantid::Kernel::V3D(); + }qt/python/instrumentview/instrumentview/test/test_model.py (2)
129-138: Projection option refactor: tests updated correctlySwitch to constants and new SideBySide path looks good.
139-149: Helper refactor is soundPassing projection_option through to calculate_projection aligns with the new API.
qt/python/instrumentview/instrumentview/Projections/SphericalProjection.py (1)
14-25: Safer spherical projection math; API alignment looks goodParameterless _calculate_2d_coordinates and clipped safe-division are appropriate.
Framework/PythonInterface/test/python/mantid/api/PanelsSurfaceCalculatorTest.py (1)
77-81: LGTM: quaternion usage is now correct.Passing [w, x, y, z] derived from Quat addresses the earlier angle-axis issue.
qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py (3)
60-61: Good: disabled logging on CreateDetectorTable.Reduces noise and overhead when building the view.
119-122: Property now returns full array; confirm downstream consumers expect 2D-only coordinates.If any consumer expects (N,3), ensure contract is clear.
230-231: Assign only the 2D positions slice — OK.Assuming the suggested (N,2) buffer change is adopted, drop the slice to a direct assignment.
Framework/PythonInterface/mantid/api/src/Exports/PanelsSurfaceCalculator.cpp (2)
73-82: Bug: calculateBankNormal returns [V3D] instead of [x, y, z].Match Python expectations by unpacking components.
Apply this diff:
- const auto normal = self.calculateBankNormal(*(cInfoSharedPtr.get()), tubesVector); - list normalList{normal}; - return normalList; + const auto normal = self.calculateBankNormal(*(cInfoSharedPtr.get()), tubesVector); + list normalList; + normalList.append(normal.X()); + normalList.append(normal.Y()); + normalList.append(normal.Z()); + return normalList;
48-60: Bug: calculatePanelNormal returns a list wrapping a V3D, not [x, y, z].Return a 3-element list of doubles; current code will break numpy assertions.
Apply this diff:
- const auto panelNormal = self.calculatePanelNormal(panelCornersVec); - list panelNormalList(panelNormal); - return panelNormalList; + const auto panelNormal = self.calculatePanelNormal(panelCornersVec); + list panelNormalList; + panelNormalList.append(panelNormal.X()); + panelNormalList.append(panelNormal.Y()); + panelNormalList.append(panelNormal.Z()); + return panelNormalList;qt/python/instrumentview/instrumentview/Projections/Projection.py (1)
59-67: Signature change ripple check — no stale implementations foundSphericalProjection.py, CylindricalProjection.py and SideBySide.py implement _calculate_2d_coordinates with the new parameterless signature; Projection._calculate_detector_coordinates calls it without args and tests/mock patches target these methods.
qt/python/instrumentview/instrumentview/Projections/SideBySide.py (8)
18-30: Good: FlatBankInfo uses safe instance defaults (dataclass + default_factory).This eliminates shared mutable state and aligns with best practices.
73-76: Good: Move mutable attributes to instance scope._state is now per‑instance (_detector_id_to_flat_bank_map, _flat_banks), avoiding cross‑instance leakage.
241-257: Good: banks_per_row is int; layout loop uses modulo safely.This resolves float modulo errors and ensures stable layout.
91-96: Pass z, x, y to setupBasisAxes; also silence unused arg warning.C++ signature is setupBasisAxes(zaxis, xaxis, yaxis). Current call passes (x, y, z).
Apply this diff:
- def _calculate_axes(self, root_position: np.ndarray) -> None: + def _calculate_axes(self, _root_position: np.ndarray) -> None: x = [0.0] * 3 y = [0.0] * 3 - self._calculator.setupBasisAxes(x, y, list(self._projection_axis)) + # z, x, y + self._calculator.setupBasisAxes(list(self._projection_axis), x, y) self._x_axis = x self._y_axis = y
130-146: Handle Optional V2D from getSideBySideViewPos and avoid KeyError for partial selections.
- Don’t assume the bank’s first detector is in the selected subset; choose any selected ID.
- Treat getSideBySideViewPos as Optional[V2D] (None when absent), not a (bool, value) pair.
Apply this diff:
- parent_component_index = component_info.parent(int(self._detector_id_component_index_map[flat_bank.detector_ids[0]])) - override_pos = self._calculator.getSideBySideViewPos(component_info, instrument, parent_component_index) - flat_bank.has_position_in_idf = override_pos[0] - if flat_bank.has_position_in_idf: - flat_bank.reference_position = np.array(override_pos[1] + [0]) + # Use any detector from this bank that is in the requested subset + selected_ids = [d for d in flat_bank.detector_ids if d in self._detector_id_component_index_map] + if selected_ids: + parent_component_index = component_info.parent( + int(self._detector_id_component_index_map[selected_ids[0]]) + ) + override_pos = self._calculator.getSideBySideViewPos(component_info, instrument, parent_component_index) + flat_bank.has_position_in_idf = override_pos is not None + if flat_bank.has_position_in_idf: + # V2D -> [x, y, 0] + flat_bank.reference_position = np.array([override_pos.X(), override_pos.Y(), 0.0])
156-160: Align tube detector IDs and positions to prevent mis-mapping.Compute positions only for selected component indices so relative_projected_positions aligns 1:1 with flat_bank.detector_ids; skip empty selections.
Apply this diff:
detector_component_indices = np.concatenate([component_info.children(p) for p in group]) detector_component_indices.sort() - detectors = [ - self._component_index_detector_id_map[i] for i in detector_component_indices if i in self._component_index_detector_id_map - ] + selected_component_indices = [i for i in detector_component_indices if i in self._component_index_detector_id_map] + if not selected_component_indices: + continue + detectors = [self._component_index_detector_id_map[i] for i in selected_component_indices] @@ - flat_bank.relative_projected_positions = self._calculate_projected_positions( - np.array([component_info.position(int(d)) for d in detector_component_indices]), normal, flat_bank - ) + flat_bank.relative_projected_positions = self._calculate_projected_positions( + np.array([component_info.position(int(i)) for i in selected_component_indices]), + normal, + flat_bank, + )Also applies to: 175-177
165-174: Use Quat accessors and SciPy order [x, y, z, w] for calcBankRotation.Indexing rotation like a list is unsafe; use imagI/J/K and real().
Apply this diff:
- rotation = self._calculator.calcBankRotation( + rot_quat = self._calculator.calcBankRotation( list(flat_bank.reference_position), list(normal), list(self._projection_axis), list(self._y_axis), list(self._sample_position), ) - # SciPy from_quat expects (x, y, z, w) - flat_bank.rotation = Rotation.from_quat([rotation[1], rotation[2], rotation[3], rotation[0]]) + # SciPy from_quat expects [x, y, z, w] + flat_bank.rotation = Rotation.from_quat( + [rot_quat.imagI(), rot_quat.imagJ(), rot_quat.imagK(), rot_quat.real()] + )
181-184: Handle Optional V2D for tube banks’ overrides.Treat return as Optional and construct [x, y, 0] when present.
Apply this diff:
- override_pos = self._calculator.getSideBySideViewPos(component_info, self._workspace.getInstrument(), group[0]) - flat_bank.has_position_in_idf = override_pos[0] - if flat_bank.has_position_in_idf: - flat_bank.reference_position = np.array(override_pos[1] + [0]) + override_pos = self._calculator.getSideBySideViewPos(component_info, self._workspace.getInstrument(), group[0]) + flat_bank.has_position_in_idf = override_pos is not None + if flat_bank.has_position_in_idf: + flat_bank.reference_position = np.array([override_pos.X(), override_pos.Y(), 0.0])
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
Mock method in a test was returning the wrong type of object, and did a couple of improvements from code review.
boost::python::len returns a signed int, so we need to convert to a size_t before comparisons in the loops. boost::python::extract generates a few maybe-uninitialized warnings with gcc.
jclarkeSTFC
force-pushed
the
side-by-side-projection
branch
from
d1d251c to
29b6bfe
Compare
Adds the "side-by-side projection" to the new Instrument View.
Closes #38429.
I've moved the code I could out from the old class,
PanelsSurfaceinto a new calculator class,PanelsSurfaceCalculator, which allowed me to reuse it in the new Instrument View.The legend is now draggable and resizeable.
Detectors in grids will be drawn as such, and any other types of detector will be grouped into a separate grid, with their position in the grid abstract and not related to their physical position (it will arrange them as close as possible to a square).
The panel rotation problem mentioned by Richard in #38429 here seems to be fixed.
The ability to flip and rotate panels is something we can add in a future PR, see here
To test:
The side-by-side option should appear as one of the projection options.
Reviewer
Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.
As per the review guidelines:
Gatekeeper
As per the gatekeeping guidelines: