Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider phase offset for disk on Merlin in PyChop #37188

Merged
merged 13 commits into from May 2, 2024

Conversation

SilkeSchomann
Copy link
Contributor

@SilkeSchomann SilkeSchomann commented Apr 19, 2024

Description of work

After changes to the disk chopper controller on MERLIN there was a mismatch between the chopper phase delay set by the IBEX controller software and PyChop.

Summary of work

In this PR the following changes were made in PyChop:

  1. The default value for the Disk chopper phase delay time was changed from 1500 to 17000.
  2. The phase offset used by the IBEX controller (currently 4500 us) is applied in PyChop when calculating the possible incident energies for a given chopper setup.
  3. The value for the phase offset is read from merlin.yaml to allow easier adjustment. For this, a new parameter called phaseOffset was introduced.
  4. I've added a unit test for validating the new parameter.
  5. There were a few deprecation warnings regarding NumPy that are fixed now as well.

Fixes #37045

Report to: Duc Le

To test:

Follow the instructions in the original GitHub issue. Please note that the default value for the
Disk chopper phase delay time has changed from 1500 to 17000.

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.

@SilkeSchomann SilkeSchomann added ISIS Team: Excitations/Vesuvio Issue and pull requests managed by the Excitiations/Vesuvio subteam at ISIS Direct Inelastic Issues and pull requests related to direct inelastic labels Apr 26, 2024
@SilkeSchomann SilkeSchomann added this to the Release 6.10 milestone Apr 26, 2024
@SilkeSchomann SilkeSchomann marked this pull request as ready for review April 26, 2024 07:53
Copy link
Member

@mducle mducle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SilkeSchomann Thanks for this! The code looks good and I tested it and it works fine, but actually the definition of the Merlin disc chopper in PyChop is completely wrong - I think I didn't have access to the engineering drawings for the chopper when I wrote the yaml file so made a best guess. In fact R. Bewley has now given the drawing and the merlin.yaml file and some other should be changed like this:

diff --git a/qt/python/mantidqtinterfaces/mantidqtinterfaces/PyChop/PyChopGui.py b/qt/python/mantidqtinterfaces/mantidqtinterfaces/PyChop/PyChopGui.py
index 7bc4a828c15..ad18745fdae 100644
--- a/qt/python/mantidqtinterfaces/mantidqtinterfaces/PyChop/PyChopGui.py
+++ b/qt/python/mantidqtinterfaces/mantidqtinterfaces/PyChop/PyChopGui.py
@@ -241,7 +241,7 @@ class PyChopGui(QMainWindow):
                 self.widgets["MultiRepCheck"].setEnabled(True)
                 self.tabs.setTabEnabled(self.tdtabID, True)
                 if self.instSciAct.isChecked():
-                    self.widgets["Chopper0Phase"]["Edit"].setText("17000")
+                    self.widgets["Chopper0Phase"]["Edit"].setText("12400")
                     self.widgets["Chopper0Phase"]["Label"].setText("Disk chopper phase delay time")
                     self.widgets["Chopper0Phase"]["Edit"].show()
                     self.widgets["Chopper0Phase"]["Label"].show()
diff --git a/scripts/pychop/ISISDisk.py b/scripts/pychop/ISISDisk.py
index f58d454efc9..02c53506df5 100644
--- a/scripts/pychop/ISISDisk.py
+++ b/scripts/pychop/ISISDisk.py
@@ -72,10 +72,11 @@ class ISISDisk:
             self.Chop2Phase = 5  # Phase delay time in usec for chopper 2 (T0/frame overlap chopper)
         elif "MERLIN" in instname:
             self.dist = [9.3, 10.1]
-            self.nslot = [1, 2]
-            self.slot_width = [950, 10]
+            self.nslot = [10, 2]
+            self.slot_width = [68, 10]
             self.guide_width = [64, 10]
-            self.radius = [250, 290]
+            self.slot_ang_pos = [[-45.5, -32.5, -19.5, -6.5, 6.5, 19.5, 32.5, 45.5, 50, 150], [0, 180]]
+            self.radius = [300, 290]
             self.numDisk = [1, 1]
             self.samp_det = 2.5
             self.chop_samp = 1.82
diff --git a/scripts/pychop/merlin.yaml b/scripts/pychop/merlin.yaml
index d7cb7b62186..4f9b776a9cf 100644
--- a/scripts/pychop/merlin.yaml
+++ b/scripts/pychop/merlin.yaml
@@ -11,18 +11,19 @@ chopper_system:
     -                           # Each entry must have a dash on an otherwise empty line!
       name: MERLIN Disk
       distance: 9.3             # Distance from moderator to this chopper in metres
-      slot_width: 950           # Slot width in mm
+      slot_width: 68            # Slot width in mm
       guide_width: 64           # Width of guide after chopper in mm
-      nslot: 1                  # Number of slots. If slot_ang_pos is specified can omit this entry
+      nslot: 10                 # Number of slots. If slot_ang_pos is specified can omit this entry
                                 # Next line has the angular position of the slots in degrees
                                 #   Must be monotonically increasing. Can omit if nslot is specified,
                                 #   in which case it will be assumed that the slits are equally spaced.
-      radius: 250               # Disk radius
+      slot_ang_pos: [-45.5, -32.5, -19.5, -6.5, 6.5, 19.5, 32.5, 45.5, 50, 150]
+      radius: 300               # Disk radius
       isDouble: False           # Is this a double disk system?
       isPhaseIndependent: True  # Is this disk to be phased independently?
-      defaultPhase: 17000        # What is the default phase for this disk (either a time in microseconds
+      defaultPhase: 12800       # What is the default phase for this disk (either a time in microseconds
                                 #   or a slot index [as a string] for the desired rep to go through)
-      phaseOffset: 4500         # Offset introduced after disk chopper controller update
+      phaseOffset: 11300        # Offset introduced after disk chopper controller update
       phaseName: 'Disk chopper phase delay time'
     -
       name: MERLIN Fermi

The offset and default phase is also different from what I said in the issue (sorry). The offset is 11300 microseconds and the default phase is 12400 microseconds. The sense of rotation is correct as you have it though (subtract the offset).

The MERLIN disc is physically made of two slots of different widths. One slot is 570mm wide (~109 degrees) whilst the other is 68mm. PyChop doesn't allow different sized slots so I've basically made the slots all 68mm wide but stacked 9 of them together to make an effect wide slot.

Copy link
Member

@mducle mducle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Works well now. Sorry for the delay. :shipit:

@robertapplin robertapplin merged commit 59da4f1 into main May 2, 2024
9 checks passed
@robertapplin robertapplin deleted the 37045_phase_offset_merlin_in_pychop branch May 2, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Direct Inelastic Issues and pull requests related to direct inelastic ISIS Team: Excitations/Vesuvio Issue and pull requests managed by the Excitiations/Vesuvio subteam at ISIS
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

PyChop - incorrect phase offset for disk on MERLIN
3 participants