Remove AlignDetectors algorithm #37799

sf1919 · 2024-08-14T09:34:26Z

Description of work

Removal of AlignDetectors Algorithm as this has been replaced with ConvertUnits.

Summary of work

Part of #31258

Further detail of work

Deprecated in 2021 - see #30163

To test:

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

Is the code of an acceptable quality?
Does the code conform to the coding standards?
Are the unit tests small and test the class in isolation?
If there is GUI work does it follow the GUI standards?
If there are changes in the release notes then do they describe the changes appropriately?
Do the release notes conform to the release notes guide?

Functional Tests

Do changes function as described? Add comments below that describe the tests performed?
Do the changes handle unexpected situations, e.g. bad input?
Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

sf1919 · 2024-08-14T11:40:06Z

@RichardWaiteSTFC can you examine the commits c3b9e0d and b25ac52 please? I'm not sure if I've made quite the right adjustments in the documentation on these.

RichardWaiteSTFC · 2024-08-14T11:52:43Z

docs/source/algorithms/EnggFitPeaks-v1.rst

+:math:`TZERO` is related to the difference between the measured and
+actual time-of-flight base on emission time from the moderator, :math:`DIFA` is an empirical term (ideally zero), and :math:`DIFC` is
+
+.. math:: DIFC = \frac{2m_N}{h} L_{tot} sin \theta


I think this info should actually live in unit factory docs
https://docs.mantidproject.org/nightly/concepts/UnitFactory.html#unit-factory
or in the ConvertUnits docs.
This algorithm is no longer used (it pre-dates my work on engineering GUI) - also the name suggest something engineering specific whereas this relates to TOF powder diffraction.

Does ConvertUnits actually use DIFC? I can't see an obvious place to put it in the documentation.

It does yes, it uses the UnitParams map in the conversions - I think the logic is in Unit.cpp - by default DIFA=TZERO=0 and DIFC is calculated using L1+L2 and sin(theta)

RichardWaiteSTFC · 2024-08-14T11:54:27Z

docs/source/algorithms/PDCalibration-v1.rst

 The resulting calibration table can be saved with
 :ref:`algm-SaveDiffCal`, loaded with :ref:`algm-LoadDiffCal` and
-applied to a workspace with :ref:`algm-AlignDetectors`. There are also
+applied to a workspace with :ref:`algm-ConvertUnits`. There are also


I think we should also mention ApplyDiffCal - which actually uses the calibration table and updates the diffractometer constants in the UnitParams map of the workspace which is then used in ConvertUnits

sf1919 · 2024-08-22T14:36:58Z

Currently blocked by #37859

sf1919 · 2024-09-03T12:26:40Z

Some workflow diagrams refer to AlignDetectors algorithm. However these appear to be generally out of date and so separate issues have been raised to deal with these (see #37905 and #37907)

sf1919 · 2024-09-10T09:31:33Z

I'm moving this to 6.12 as there are some issues that I think need to be fixed separately before this one can continue. I'll write up the relevant issues.

github-actions · 2024-12-06T14:12:35Z

👋 Hi, @sf1919,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

- Remove head and main files - Remove references from CMake

- Remove test and reference from CMake

- Removed the User documentation of the algorithm - Removed links from previous release notes - Removed references to the AlignDetectors Algorithm as an example in documentation

- Where possible remove or convert reference to AlignDetectors to ConvertUnits

- For some algorithm documentation there are references made to the GSAS equations in AlignDetectors. This commit moves that information into the relevant documentation to enable the references to AlignDetectorsto be removed.

- The GSAS equation information would be best situated in the Unit Factory concept documentation. This commit makes that change and updates references to this concept documentation instead.

- Remove references to AlignDetectors from CalFile and PowderDiffractionCalibration. - Replace references where necessary with suitable alternatives

- DiffractionFocussing2 made reference to using AlignDetectors. This has been changed to ConvertUnits - The associated test used AlignDetectors. This has been changed to ConvertUnits and variables renamed accordingly - Updated DiffractionFocussing2 documentation

Even though the reference is the focussed d-spacing spectrum (so no conversion back to TOF required), the test was failing because the parameter map in the two workspaces was different.

docs/source/concepts/calibration/PowderDiffractionCalibration.rst

RichardWaiteSTFC · 2025-01-31T10:02:52Z

scripts/Diffraction/isis_powder/gem_routines/gem_calibration_algs.py

    )
    rebinned_tof = mantid.ConvertUnits(InputWorkspace=rebinned, Target="TOF")
-    aligned = mantid.AlignDetectors(InputWorkspace=rebinned_tof, CalibrationFile=offset_file)
+    mantid.ApplyDiffCal(InstrumentWorkspace="rebinned_tof", CalibrationFile=offset_file)


It would actually be better to use the offsets workspace (OffsetsWorkspace=offset_ws_name here). The act of writing to a fixed width column can truncate a number, which in this case can lead to quite large deviations in the data (certainly the y-value in a given bin) relative to typical absolute tolerances used in system tests because the y-values are large and the peak is quite sharp.

Even though the d-spacing of the workspaces calibrated using the file and the offsets workspace differ only by ~1e-8

I have kept the use of the offsets file so as to 'fix' the test and preserve the behaviour - in practice this difference in offset used will never be noticed in the refinement, it is only noticeable in system tests. I think the tests interpret the bin edges as matching within tolerance (certainly Minus works even though the edges differ by ~1e-8)?

andy-bridger · 2025-02-04T10:47:32Z

docs/source/concepts/calibration/PowderDiffractionCalibration.rst


 1. Load the calibration data using :ref:`Load <algm-Load>`
-2. Run :ref:`AlignDetectors <algm-AlignDetectors>`, this will convert the data to d-spacing and apply the calibration.  You can provide the calibration using the ``CalibrationFile``, the ``CalibrationWorkspace``, or ``OffsetsWorkspace``.
+2. Run :ref:`ConvertUnits <algm-ConvertUnits>`, this will convert the data to d-spacing and apply the calibration.  You can provide the calibration using the ``CalibrationFile``, the ``CalibrationWorkspace``, or ``OffsetsWorkspace``.


Is this true? I thought ConvertUnits just converted it to d-spacing and the calibration would have to be applied with ApplyDiffCal?

I agree I think there's a missing step to call ApplyDiffCal which updates the parameter map on the workspace (i.e. the information used to convert between TOF -> d-spacing) then ConvertUnits will use this calibration. Previously AlignDetectors would apply the calibration and then perform the conversion from TOF -> d-spacing but there would be no update of the parameters on the workspace, such that the inverse conversion (from d-spacing -> TOF) would use the un-calibrated DIFC parameter.

I think it would also be nice to have that clarified in the docs (docs/source/algorithms/ConvertUnits-v1.rst) because I don't think it currently mentions anything about calibration

andy-bridger

Not sure how the review functionality works so whether this needs to be separate, but if we could maybe get these changes (#37799 (comment)) to docs/source/concepts/calibration/PowderDiffractionCalibration.rst and docs/source/algorithms/ConvertUnits-v1.rst

RichardWaiteSTFC · 2025-02-04T13:58:09Z

Thanks @andy-bridger for the review, I've added the section you requested in ConvertUnits and clarified the role of ApplyDiffCal in PowderDiffractionCalibration.rst and a few other places that I could find.

Just a note when reviewing, anywhere in the code where AlignDetectors has been replaced there needs to be two calls to ApplyDiffCal - the first with the calibration file/workspace, the second call after ConvertUnits with ClearCalibration=True - this means the calibration won't be used to convert back from d-spacing to TOF (which was the behaviour of AlignDetectors).
I think I fixed this when I fixed the systems tests, but I'd appreciate a second pair of eyes on it!

andy-bridger · 2025-02-04T14:14:58Z

Framework/Algorithms/test/DiffractionFocussing2Test.h

No second call of ApplyDiffCal to ClearCalibration here, but I'm guessing that is fine because it is just a test and there is no further processing?

Yep I think that's OK because it's checking the focussing produces a workspace with a single histogram - technically the DIFC of the focussed spectrum will be different but that's not part of what is being tested there

Also clarify role of ConvertUnits and ApplyDiffCal in other places in docs

andy-bridger · 2025-02-04T14:20:25Z

docs/source/algorithms/DiffractionFocussing-v2.rst

again I assume it is intentional to the example on L83 that ApplyDiffCal is not called with ClearCalibration?

Yep I think that's OK as well because the data are not converted back to TOF in that usage example

andy-bridger

All looks good now

cailafinn

Changes looking good, just a couple of quick Q's about the documentation around them:

Framework/Algorithms/inc/MantidAlgorithms/DiffractionFocussing2.h

Framework/Algorithms/inc/MantidAlgorithms/ReadGroupsFromFile.h

docs/source/algorithms/ConvertUnits-v1.rst

docs/source/concepts/calibration/PowderDiffractionCalibration.rst

sf1919 · 2025-02-07T11:25:45Z

It doesn't look like we've heard back about VULCAN so I've asked @peterfpeterson for help

RichardWaiteSTFC · 2025-02-07T11:44:29Z

It doesn't look like we've heard back about VULCAN so I've asked @peterfpeterson for help

ApplyDiffCal calls LoadDiffCal if a calibration file is provided so I'm pretty confident it's a like-for-like swap!

mantid/Framework/DataHandling/src/ApplyDiffCal.cpp

Lines 150 to 154 in d1e96c3

    
           const std::string calFileName = getPropertyValue("CalibrationFile"); 
        
           if (!calFileName.empty()) { 
        
             progress(0.0, "Reading calibration file"); 
        
             loadCalFile(inputWS, calFileName); 
        
             return;

mantid/Framework/DataHandling/src/ApplyDiffCal.cpp

Lines 122 to 123 in d1e96c3

    
           void ApplyDiffCal::loadCalFile(const Workspace_sptr &inputWS, const std::string &filename) { 
        
             auto alg = createChildAlgorithm("LoadDiffCal");

The only thing that might be an issue is that the calibration is not cleared subsequently, however it looks like there are no future conversions back to TOF in the file so this doesn't matter. On second thought we might as well do it!

peterfpeterson

I'm happy if this is merged. I didn't approve so as to not step on toes.

Co-authored-by: Caila Finn <47181718+cailafinn@users.noreply.github.com>

* Remove AlignDetectors algorithm - Remove head and main files - Remove references from CMake * Remove aligndetectorstest - Remove test and reference from CMake * Remove user documentation - Removed the User documentation of the algorithm - Removed links from previous release notes - Removed references to the AlignDetectors Algorithm as an example in documentation * Remove references for AlignDetectors in algorithm documentation - Where possible remove or convert reference to AlignDetectors to ConvertUnits * Update algorithm documentation with maths from AlignDetectors - For some algorithm documentation there are references made to the GSAS equations in AlignDetectors. This commit moves that information into the relevant documentation to enable the references to AlignDetectorsto be removed. * Move GSAS equation information - The GSAS equation information would be best situated in the Unit Factory concept documentation. This commit makes that change and updates references to this concept documentation instead. * Add mention of ApplyDiffCal algorithm in PDCalibration documentation * Update calibration concept documentation - Remove references to AlignDetectors from CalFile and PowderDiffractionCalibration. - Replace references where necessary with suitable alternatives * Update DiffractionFocussing2 - DiffractionFocussing2 made reference to using AlignDetectors. This has been changed to ConvertUnits - The associated test used AlignDetectors. This has been changed to ConvertUnits and variables renamed accordingly - Updated DiffractionFocussing2 documentation * Add ApplyDIffCal to DiffractionFocussing2 - ConvertUnits is not a direct swap with AlignDetectors. Instead a combination of ApplyDIffCal and ConvertUnits is needed. This makes that fix, * Remove see also references Remove links from other algorithms for AlignDetectors under the 'see also' section * Update the gem_routines/gem_calibration_algs script - Remove AlignDetectors and replace with ApplyDiffCal and COnvertUnits algorithms * Remove AlignDetectors from WishAnalysis System test - Replaced with ApplyDiffCal and ConvertUnits algorithms * Use correct parameters for ApplyDiffCal - ApplyDiffCal does not take an InputWorkspace. Instead it takes an InstrumentWorkspace. * More documentation cleaning - Removed link to alignDetectors from See Also in SaveGDA - Removed reference from explanation of OffsetsWorkspace * Fix problems with WishAnalysis test * Fix failing system tests * Add release note * Upate imports in WishAnalyis.py test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Move release note * Fix WISH system test by clearing calibration Reference file was converted back to TOF so for backwards compatibility need to clear calibration (so uncalibrated FIC used for backward conversion) * Fix gem calibration test by clearing calibration Even though the reference is the focussed d-spacing spectrum (so no conversion back to TOF required), the test was failing because the parameter map in the two workspaces was different. * Fix doc test still using AlignDetectors * Remove references to AlignDetectors in docs and comments etc. * Add calibration section to ConvertUnits Also clarify role of ConvertUnits and ApplyDiffCal in other places in docs * Update docs/source/algorithms/ConvertUnits-v1.rst Co-authored-by: Caila Finn <47181718+cailafinn@users.noreply.github.com> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Richard Waite <richard.waite@stfc.ac.uk> Co-authored-by: Caila Finn <47181718+cailafinn@users.noreply.github.com>

sf1919 added the Framework Issues and pull requests related to components in the Framework label Aug 14, 2024

sf1919 added this to the Release 6.11 milestone Aug 14, 2024

RichardWaiteSTFC reviewed Aug 14, 2024

View reviewed changes

This was referenced Aug 22, 2024

Evaluate deprecated algorithms to remove them/de-deprecate them #31258

Closed

Update PG3StripPeaks to not use AlignDetectors algorithm #37859

Closed

sf1919 added Diffraction Issues and pull requests related to diffraction ISIS: Core Issue and pull requests at ISIS that relate to Core features and functions Extra Attention Testers and Gate keepers should pay extra attention as this affects core aspects. labels Aug 23, 2024

sf1919 assigned RichardWaiteSTFC Aug 30, 2024

This was referenced Sep 3, 2024

Update Spectroscopy Workflow files #37905

Closed

Update Powder Diffraction and Pearl Workflows #37907

Open

sf1919 force-pushed the 31258_aligndetectors branch from 3eaf334 to 34d3769 Compare September 10, 2024 07:20

sf1919 modified the milestones: Release 6.11, Release 6.12 Sep 10, 2024

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Dec 6, 2024

sf1919 added 9 commits December 10, 2024 16:32

Remove AlignDetectors algorithm

9fdde69

- Remove head and main files - Remove references from CMake

Remove aligndetectorstest

085afb0

- Remove test and reference from CMake

Remove user documentation

09537b1

- Removed the User documentation of the algorithm - Removed links from previous release notes - Removed references to the AlignDetectors Algorithm as an example in documentation

Remove references for AlignDetectors in algorithm documentation

d15d7d0

- Where possible remove or convert reference to AlignDetectors to ConvertUnits

Move GSAS equation information

5897496

- The GSAS equation information would be best situated in the Unit Factory concept documentation. This commit makes that change and updates references to this concept documentation instead.

Add mention of ApplyDiffCal algorithm in PDCalibration documentation

aef03e9

Update calibration concept documentation

3be4598

- Remove references to AlignDetectors from CalFile and PowderDiffractionCalibration. - Replace references where necessary with suitable alternatives

RichardWaiteSTFC added 3 commits January 30, 2025 18:10

Fix gem calibration test by clearing calibration

3fb3cdc

Even though the reference is the focussed d-spacing spectrum (so no conversion back to TOF required), the test was failing because the parameter map in the two workspaces was different.

Fix doc test still using AlignDetectors

47af5f5

Remove references to AlignDetectors in docs and comments etc.

e824e9e

RichardWaiteSTFC reviewed Jan 31, 2025

View reviewed changes

docs/source/concepts/calibration/PowderDiffractionCalibration.rst Show resolved Hide resolved

RichardWaiteSTFC reviewed Jan 31, 2025

View reviewed changes

sf1919 marked this pull request as ready for review January 31, 2025 12:21

sf1919 linked an issue Jan 31, 2025 that may be closed by this pull request

Evaluate deprecated algorithms to remove them/de-deprecate them #31258

Closed

8 tasks

sf1919 assigned cailafinn and andy-bridger Feb 4, 2025

andy-bridger reviewed Feb 4, 2025

View reviewed changes

andy-bridger requested changes Feb 4, 2025

View reviewed changes

andy-bridger reviewed Feb 4, 2025

View reviewed changes

Add calibration section to ConvertUnits

25a29f3

Also clarify role of ConvertUnits and ApplyDiffCal in other places in docs

andy-bridger reviewed Feb 4, 2025

View reviewed changes

RichardWaiteSTFC force-pushed the 31258_aligndetectors branch from f002f3f to 25a29f3 Compare February 4, 2025 14:30

andy-bridger previously approved these changes Feb 4, 2025

View reviewed changes

cailafinn requested changes Feb 4, 2025

View reviewed changes

peterfpeterson reviewed Feb 7, 2025

View reviewed changes

Update docs/source/algorithms/ConvertUnits-v1.rst

8303ffa

Co-authored-by: Caila Finn <47181718+cailafinn@users.noreply.github.com>

sf1919 dismissed andy-bridger’s stale review via 8303ffa February 12, 2025 09:01

cailafinn approved these changes Feb 12, 2025

View reviewed changes

cailafinn merged commit 77e176c into main Feb 12, 2025
10 checks passed

cailafinn deleted the 31258_aligndetectors branch February 12, 2025 17:38

peterfpeterson mentioned this pull request Feb 12, 2025

Remove AlignDetectors algorithm - ornl-next #38832

Merged

Remove AlignDetectors algorithm #37799

Remove AlignDetectors algorithm #37799

Uh oh!

Conversation

sf1919 commented Aug 14, 2024

Description of work

Summary of work

Further detail of work

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

Uh oh!

sf1919 commented Aug 14, 2024

Uh oh!

RichardWaiteSTFC Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

sf1919 Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

RichardWaiteSTFC Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichardWaiteSTFC Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sf1919 commented Aug 22, 2024

Uh oh!

sf1919 commented Sep 3, 2024

Uh oh!

sf1919 commented Sep 10, 2024

Uh oh!

github-actions bot commented Dec 6, 2024

Uh oh!

Uh oh!

RichardWaiteSTFC Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichardWaiteSTFC Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-bridger Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichardWaiteSTFC Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-bridger Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

andy-bridger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichardWaiteSTFC commented Feb 4, 2025

Uh oh!

andy-bridger Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichardWaiteSTFC Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

andy-bridger Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichardWaiteSTFC Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

andy-bridger left a comment

Choose a reason for hiding this comment

RichardWaiteSTFC Aug 14, 2024 •

edited

Loading

RichardWaiteSTFC Aug 14, 2024 •

edited

Loading

RichardWaiteSTFC Jan 31, 2025 •

edited

Loading

RichardWaiteSTFC Jan 31, 2025 •

edited

Loading

andy-bridger Feb 4, 2025 •

edited

Loading

RichardWaiteSTFC Feb 4, 2025 •

edited

Loading

andy-bridger left a comment •

edited

Loading

andy-bridger Feb 4, 2025 •

edited

Loading

andy-bridger Feb 4, 2025 •

edited

Loading

RichardWaiteSTFC commented Feb 7, 2025 •

edited

Loading