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

Load instrument after sample logs in ReflectometryReductionOneLiveData #37053

Conversation

rbauststfc
Copy link
Contributor

Description of work

Summary of work

This PR changes ReflectometryReductionOneLiveData so that we load the sample logs into the live data workspace that's passed into the reduction before loading the instrument. We also load the sample logs as number series types. These changes together mean that when the instrument is loaded into the workspace the theta value can be picked up by the INTER IDF and used to calculate the correct detector pixel y positions. As a result, the detector pixel theta values are set correctly and this prevents an error when running the reduction using summation in Q (which relies on correct theta values).

Fixes #37043.

Report to: Max and Becky at ISIS

Further detail of work

This fixes the main problem reported on the linked issue. As noted on the issue, I intend to continue investigating why the bank summing step (i.e. the call to ReflectometryISISSumBanks that produces the summed segment workspace) seems to be impacting the theta value used when summing in Q. The result of this investigation shouldn't impact on this fix, though, and if a further fix is required for that it will be done under a separate issue.

To test:

See instructions on linked issue. This time there should not be an error. Also, if you compare the theta values for the detectors (using Show Detectors) in the TOF_live and TOF_summed_segment workspaces then those in TOF_summed_segment should now be different.

Note that you may find you need to be plugged in on site to run live data. You will also need to have the following dependency installed in your conda environment: conda install -c paulscherrerinstitute cachannel. When running live data on Windows you can get a dialog popping up with an error about caRepeater, but this can be closed and ignored.

This does not require release notes because it will be covered by the patch release notes.


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.

@rbauststfc rbauststfc added Reflectometry Issues and pull requests related to reflectometry Patch Candidate Urgent issues that must be included in a patch following a release ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS labels Mar 21, 2024
@rbauststfc rbauststfc added this to the Release 6.9.1 milestone Mar 21, 2024
@jhaigh0
Copy link
Contributor

jhaigh0 commented Mar 21, 2024

Had this failure in the doc tests

Document: algorithms/SaveISISReflectometryORSO-v1
-------------------------------------------------
CreateSampleWorkspace-[Notice] CreateSampleWorkspace started
CreateSampleWorkspace-[Notice] CreateSampleWorkspace successful, Duration 0.00 seconds
AddSampleLog-[Notice] AddSampleLog started
AddSampleLog-[Notice] AddSampleLog successful, Duration 0.00 seconds
SaveISISReflectometryORSO-[Notice] SaveISISReflectometryORSO started
SaveISISReflectometryORSO-[Notice] SaveISISReflectometryORSO successful, Duration 0.01 seconds
**********************************************************************
File "algorithms/SaveISISReflectometryORSO-v1.rst", line 125, in SaveISISReflectometryORSO_general_usage
Failed example:
    # import the os path libraries for directory functions
    import os

    ws = CreateSampleWorkspace(XUnit="MomentumTransfer")

    # Create an absolute path by joining the proposed filename to a directory
    # os.path.expanduser("~") used in this case returns the home directory of the current user
    file = os.path.join(os.path.expanduser("~"), "ws")

    # Add Sample Log entries
    AddSampleLog(Workspace=ws, LogName='rb_proposal', LogText='1234', LogType='Number')

    # Save the ORSO file
    SaveISISReflectometryORSO(InputWorkspace=ws, Filename=file, WriteResolution=False)

    # Open the file and read the first line
    if os.path.exists(file + ".ort"):
      with open((file + ".ort"), 'r') as myFile:
        print(myFile.readline())
Expected:
    # # ORSO reflectivity data file | 1.0 standard | YAML encoding | https://www.reflectometry.org/
Got:
    # # ORSO reflectivity data file | 1.1 standard | YAML encoding | https://www.reflectometry.org/

**********************************************************************
1 items had failures:
   1 of   1 in SaveISISReflectometryORSO_general_usage
1 tests in 1 items.
0 passed and 1 failed.
***Test Failed*** 1 failures.

@rbauststfc
Copy link
Contributor Author

Ah, thanks @jhaigh0, I forgot that doctest fix had gone in since 6.9. I'll cherry pick it across here - is it affecting all the patch PRs, because I would probably expect it to?

@jhaigh0
Copy link
Contributor

jhaigh0 commented Mar 21, 2024

Ah, thanks @jhaigh0, I forgot that doctest fix had gone in since 6.9. I'll cherry pick it across here - is it affecting all the patch PRs, because I would probably expect it to?

Haven't actually had any other osx builds finidh for those PRs, we'll keep a look-out though and get this in first if it happens to the others

The orso standard version is provided by orsopy, which can update on
conda without warning. Given this, remove the specific version from the
doctest, the gist is enough.
Copy link
Contributor

@jclarkeSTFC jclarkeSTFC left a comment

Choose a reason for hiding this comment

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

Problem fixed

@rbauststfc rbauststfc marked this pull request as ready for review March 21, 2024 16:24
@thomashampson thomashampson merged commit 1b0e1a4 into mantidproject:release-next Mar 21, 2024
9 checks passed
@rbauststfc rbauststfc deleted the 37043_patch_live_data_sum_in_q branch April 24, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS Patch Candidate Urgent issues that must be included in a patch following a release Reflectometry Issues and pull requests related to reflectometry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants