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

Merge In Updated Tube Calib Scripts #27386

Closed
Tracked by #33821
DavidFair opened this issue Nov 14, 2019 · 6 comments · Fixed by #35984
Closed
Tracked by #33821

Merge In Updated Tube Calib Scripts #27386

DavidFair opened this issue Nov 14, 2019 · 6 comments · Fixed by #35984
Assignees
Labels
ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS Reported By User Issues that were found or highlighted by a user/scientist SANS Issues and pull requests related to SANS
Milestone

Comments

@DavidFair
Copy link
Contributor

DavidFair commented Nov 14, 2019

Original reporter: ISIS/Richard

Richard has been recently working on a local copy of the tube calibration scripts (scripts/calibration) and would like the changes merged back in and tidied up.

Specifically, these changes work better where the resolution is bad. However, it has been changed with the horizontal tubes of SANS2D rather than the existing tubes (horizontal on MERLIN) in mind. So we need to check that it still produces sane results on instruments with horizontal tubes.

60330front_TEST_SCRIPT.py - Exercises the two below scripts
tube_calib_RKH.py.txt
tube_RKH.py.txt

@DavidFair DavidFair added ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS SANS Issues and pull requests related to SANS labels Nov 14, 2019
@DavidFair DavidFair added this to the Release 4.3 milestone Nov 14, 2019
@DavidFair DavidFair self-assigned this Nov 14, 2019
@DavidFair DavidFair modified the milestones: Release 5.0, Release 5.1 Feb 20, 2020
@gemmaguest gemmaguest removed this from the Release 5.1 milestone Jul 14, 2020
@smk78
Copy link
Contributor

smk78 commented Jul 16, 2020

NB: Richard says start with the Direct Beam iterator script first (#28986) as it will be easier!

@gemmaguest gemmaguest added the Reported By User Issues that were found or highlighted by a user/scientist label Jul 29, 2020
@gemmaguest
Copy link
Member

See also #10892

@stale
Copy link

stale bot commented Feb 22, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you feel this is incorrect please comment to keep it alive, with a reason why.

To prevent closure, e.g. for long-term planning issues, add the "Never Stale" label.

@stale stale bot added the Stale This label is automatically applied to issues that are automatically closed by the stale bot label Feb 22, 2021
@smk78 smk78 added Never Stale and removed Stale This label is automatically applied to issues that are automatically closed by the stale bot labels Feb 22, 2021
@stale
Copy link

stale bot commented Aug 30, 2021

This issue has been automatically marked as stale because it has not had activity in 6 months. It will be closed in 7 days if no further activity occurs.
Allowing issues to close as stale helps us filter out issues which can wait for future development time. All issues closed by stale bot act like normal issues; they can be searched for, commented on or reopened at any point.
If you'd like a closed stale issue to be considered, feel free to either re-open the issue directly or contact a developer.
To extend the lifetime of an issue please comment below, it helps us see that this is still affecting you and you want it fixed in the near-future. Extending the lifetime of an issue may cause the development team to prioritise it over other issues, which may be closed as stale instead.

@stale stale bot added the Stale This label is automatically applied to issues that are automatically closed by the stale bot label Aug 30, 2021
@stale
Copy link

stale bot commented Sep 6, 2021

This issue has been closed automatically. If this still affects you please re-open this issue with a comment or contact us so we can look into resolving it.

@stale stale bot closed this as completed Sep 6, 2021
@gemmaguest gemmaguest reopened this May 5, 2022
@stale stale bot removed the Stale This label is automatically applied to issues that are automatically closed by the stale bot label May 5, 2022
@gemmaguest gemmaguest added this to the Release 6.5 S1 milestone Jul 18, 2022
@gemmaguest gemmaguest removed this from the Release 6.5 S1 milestone Aug 23, 2022
@gemmaguest gemmaguest added this to the Release 6.6 milestone Oct 18, 2022
@gemmaguest gemmaguest removed this from the Release 6.6 milestone Jan 17, 2023
@rbauststfc rbauststfc assigned rbauststfc and unassigned rprospero Feb 20, 2023
@rbauststfc rbauststfc added this to the Release 6.7 milestone Feb 20, 2023
@rbauststfc
Copy link
Contributor

rbauststfc commented Mar 14, 2023

Outstanding tasks as of 14/03/2023:

  • Once we are sure that the algorithm is producing a correct result, we should add some automated tests and merge the script into the Mantid codebase. This will allow us to make further changes while remaining confident that functionality hasn't been broken.
  • Check the handling of skipped tubes in the diagnostic output that is created after the calibration has finished. Should we still be outputting something here for tubes that have been skipped? Think this isn't required.
  • Tidy up Background parameter code - this parameter value is actually not used at all. In the code, the FlatTopPeak algorithm is used for the peak fitting and this algorithm uses a hard-coded value as the background. We need to confirm what this parameter is actually doing and it's likely we should make this use the value passed in via the Background parameter. The other place in the code that uses a background value is if a Gaussian function is used to fit the peak, in which case another hard-coded value is used. However, while the code for Gaussian fitting is still in the script, we’re not actually using it so I think we could remove it.
  • Calibrations for both Larmor and Zoom need to be supported by the script. Both Larmor and Zoom have only rear detectors. Not currently a requirement - a new issue will be opened in future if this is still needed.
  • Refactor the code. In particular it looks like there could be quite a bit of unused code in the script. It would be good to merge it all in initially so that we have a copy of the original working version, but then we should remove unused blocks of code and streamline where possible to improve readability and maintainability.
  • Improve error handling in the code.
  • Add documentation for the new algorithms.

Other outstanding tasks have been moved to the following separate issues:

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 Reported By User Issues that were found or highlighted by a user/scientist SANS Issues and pull requests related to SANS
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants