Add and remove peaks in new Instrument View #40508

jclarkeSTFC · 2025-12-17T16:57:01Z

Add and remove peaks in the new Instrument View. If a single detector is selected then one can now select Add Peak and select the peak location in the line plot. If a single peaks workspace is already selected to overlay then the peak will be added to that workspace, otherwise it will be added to a separate Instrument View peaks workspace. When one or more detectors are selected, one can select Delete Single Peak and click in the line plot, then the peak closest to where you clicked will be removed from its peaks workspace. The final option is to delete all peaks on all selected detectors.

Closes #40045 .

To test:

Add peaks, delete peaks (including in the old Instrument View, I suggest having both open), do some masking at the same time. Use the old Instrument View to check that peaks have been added and removed correctly.

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:

Is the code of an acceptable quality? (Code standards/GUI standards)
Has a thorough functional test been performed? Do the changes handle unexpected input/situations?
Are appropriately scoped unit and/or system tests provided?
Do the release notes conform to the guidelines and describe the changes appropriately?
Has the relevant (user and developer) documentation been added/updated?
If the PR author isn’t in the mantid-developers or mantid-contributors teams, add a review comment rerun ci to authorize/rerun the CI

Gatekeeper

As per the gatekeeping guidelines:

Has a thorough first line review been conducted, including functional testing?
At a high-level, is the code quality sufficient?
Are the base, milestone and labels correct?

If one peaks workspace is selected in the list then add peaks to it, otherwise add peaks in a special workspace like the old instrument view.

This will delete the peak nearest to the place where the user clicks in the line plot.

Every peak on any selected detector will be deleted from the respective peaks workspace. Exposed removePeaks() to Python so I could remove multiple peaks at once. This makes it easier because otherwise after every peak removal I would have to recalculate the peak indices, plus the interface would be refreshed on the peaks workspace changing.

Calling CreatePeaksWorkspace without StoreInADS=False will result the add callback on the ADS observer being called before CreatePeaksWorkspace has exited. This will end up calling a different algorithm which cannot proceed because CreatePeaksWorkspace is still running, hence a deadlock. The fix is to add the peaks workspace manually, then CreatePeaksWorkspace won't be running and other algorithms will be free to run.

RichardWaiteSTFC

Thanks for this - I've had a look through but not tested yet sorry!

qt/python/instrumentview/instrumentview/ConvertUnitsCalculator.py

qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py

RichardWaiteSTFC · 2026-01-06T16:30:37Z

qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py

+            return ads.retrieveWorkspaces(selected_peaks_workspaces)[0]
+        if self._instrument_view_peaks_ws_name in ads.getObjectNames():
+            return ads.retrieveWorkspaces([self._instrument_view_peaks_ws_name])[0]
+        peaks_ws = CreatePeaksWorkspace(self._workspace, 0, OutputWorkspace=self._instrument_view_peaks_ws_name, StoreInADS=False)


Why not set StoreInADS=True as you add it to ADS in the next line? If you do need to keep it out of the ADS then I think with StoreInADS=False you don't need to specify OutputWorkspace

This is because with StoreInADS=True it will trigger the ADS callbacks before the algorithm has exited, causing a hang.

RichardWaiteSTFC · 2026-01-06T16:40:44Z

qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py

+            if peaks_ws.name() not in peaks_grouped_by_ws:
+                continue
+            peaks_by_detector = peaks_grouped_by_ws[peaks_ws.name()]
+            picked_detector_peaks = [p for p in peaks_by_detector if p.detector_id in detector_ids]


The peaks workspace has a DetID column which means this search for matching detector indices could be vectorised? It might not make much difference!

Bit of a big refactor to do that, can be tested out another time I think

RichardWaiteSTFC · 2026-01-06T16:49:50Z

qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py

+
+    def _update_peak_buttons(self) -> None:
+        self._view.set_add_peak_button_enabled(
+            len(self._model.picked_detector_ids) == 1 and self._peak_interaction_status != PeakInteractionStatus.Adding


I'm confused why we need the check for self._peak_interaction_status != PeakInteractionStatus.Adding - surely if this is set then the add button should be/is enabled? Is this because you expect it to be in the state PeakInteractionStatus.Disabled?

If you click the add button then the button is disabled until you've selected the peak in the line plot. When going into this method it could be either adding or deleting peaks.

RichardWaiteSTFC · 2026-01-06T16:51:16Z

qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py

    def closeEvent(self, QCloseEvent: QEvent) -> None:
        """When closing, make sure to close the plotters and figure correctly to prevent errors"""
        super().closeEvent(QCloseEvent)
+        with suppress(TypeError):


This looks like it fixes an unrelated bug - does it require a separate release note?

Never actually ran into this bug, this is just bombproofing.

qt/python/instrumentview/instrumentview/test/test_convert_units_calculator.py

These are updated by the calibration, and will give different numbers to the values from the detector info. Co-authored-by: RichardWaiteSTFC

Co-authored-by: RichardWaiteSTFC

Co-authored-by: <RichardWaiteSTFC>

RichardWaiteSTFC

Sorry, small bug with unit conversion still!

Also a usability thing - you often need to zoom into a peak on 1D plot before adding. Here you have to remember to do that before clicking Add.
If you do zoom into the 1d plot but leave the zoom button clicked and add a peak the red line doesn't show up in the plot - is that easy to fix?

RichardWaiteSTFC · 2026-01-08T11:54:33Z

qt/python/instrumentview/instrumentview/ConvertUnitsCalculator.py

+        if source_unit == target_unit:
+            return value
+        diffraction_constants = self._spectrum_info.diffractometerConstants(int(spectrum_no))
+        return UnitConversion.run(source_unit, target_unit, value, self._l1, DeltaEModeType.Elastic, diffraction_constants)


As per previous comment (albeit edited yesterday - sorry for not drawing your attention to it earlier) I think for conversion TOF <-> wavelength you still need L2 as well!

I now see this error when x unit of 1D plot set to wavelength and workspace has units of TOF.

RuntimeError: An l2 value must be supplied in the extra parameters when initialising Wavelength for conversion via TOF

FYI For conversion between wavelength <-> d-spacing you'd think it needs the two-theta, but it looks like it goes via TOF because I get the same error above with a workspace in wavelength and 1D plot in d-spacing and vice versa. So L2 alone should suffice!

Thanks for spotting this, fixed now hopefully.

RichardWaiteSTFC · 2026-01-08T12:02:44Z

qt/python/instrumentview/instrumentview/FullInstrumentViewModel.py

        self._workspace_x_unit = x_unit.unitID()
        self._workspace_x_unit_display = f"{str(x_unit.caption())} ({str(x_unit.symbol())})"
        self._selected_peaks_workspaces = []
+        self._instrument_view_peaks_ws_name = f"instrument_view_peaks_{self._workspace.name()}"


Is there any reason to preserve backward compatibility by naming the workspace SingleCrystalPeakTable?
I do like however the table doesn't overwrite a table produced on another workspace.

I don't know of a specific reason

Trying to zoom or do anything else is not possible because the interface is waiting for the peak to be picked.

If the user is in the middle of adding a peak, then they select a detector, then stop the adding peak process. You can only add a peak if exactly one detector is selected, otherwise we don't know which detector the peak is on.

jclarkeSTFC · 2026-01-09T14:01:57Z

Sorry, small bug with unit conversion still!

Also a usability thing - you often need to zoom into a peak on 1D plot before adding. Here you have to remember to do that before clicking Add. If you do zoom into the 1d plot but leave the zoom button clicked and add a peak the red line doesn't show up in the plot - is that easy to fix?

It looks to me like the red line doesn't show up, but it does add the peak in the correct place. I've changed it slightly so that if you click 'Add Peak' then it disables the toolbar until the peak is added, otherwise could be some odd behaviour if the zoom tool is selected with the red line enabled as well. For getting the red line to appear while zoomed in, wasn't immediately obvious how to fix that, perhaps a separate PR?

jclarkeSTFC added 6 commits December 17, 2025 16:48

Add peak adding functionality

b84a225

If one peaks workspace is selected in the list then add peaks to it, otherwise add peaks in a special workspace like the old instrument view.

Add delete single peak functionality

b97ba7d

This will delete the peak nearest to the place where the user clicks in the line plot.

Add tests for new adding/deleting peaks functionality

b2f9a02

Suppress error in case of no connection

2973a75

jclarkeSTFC added this to the Release 6.15 milestone Dec 17, 2025

jclarkeSTFC added the Epic Used for issues and PRs that are managed under the ISIS Epic Workstream label Dec 17, 2025

Add release note

f45f702

jclarkeSTFC marked this pull request as ready for review December 18, 2025 09:38

jclarkeSTFC requested a review from GuiMacielPereira December 18, 2025 12:48

RichardWaiteSTFC self-assigned this Jan 5, 2026

RichardWaiteSTFC requested changes Jan 6, 2026

View reviewed changes

jclarkeSTFC added 4 commits January 7, 2026 08:50

Merge branch 'main' into 40045-add-remove-peaks

343102c

Use diffractometer constants for unit conversion

9a94c7c

These are updated by the calibration, and will give different numbers to the values from the detector info. Co-authored-by: RichardWaiteSTFC

Use ADS.doesExist instead of loop

c84f3b6

Co-authored-by: RichardWaiteSTFC

Update year in copyright notices

dc0c8f9

Co-authored-by: <RichardWaiteSTFC>

jclarkeSTFC force-pushed the 40045-add-remove-peaks branch from bce00a9 to dc0c8f9 Compare January 7, 2026 13:35

jclarkeSTFC requested a review from RichardWaiteSTFC January 7, 2026 13:41

RichardWaiteSTFC requested changes Jan 8, 2026

View reviewed changes

jclarkeSTFC added 3 commits January 9, 2026 11:17

Include l2 in unit conversion

7626ff3

Disable line plot toolbar when adding a peak

c8d36f1

Trying to zoom or do anything else is not possible because the interface is waiting for the peak to be picked.

Fix problem picking a detector while adding peak

8da230b

If the user is in the middle of adding a peak, then they select a detector, then stop the adding peak process. You can only add a peak if exactly one detector is selected, otherwise we don't know which detector the peak is on.

Add and remove peaks in new Instrument View #40508

Are you sure you want to change the base?

Add and remove peaks in new Instrument View #40508

Conversation

jclarkeSTFC commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To test:

Reviewer

Gatekeeper

Uh oh!

RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jclarkeSTFC commented Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jclarkeSTFC commented Dec 17, 2025 •

edited

Loading