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

Add additional support for saving data using the ISIS Sans Command Interface #38618

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

adriazalvarez
Copy link
Contributor

@adriazalvarez adriazalvarez commented Jan 17, 2025

Description of work

This PR modifies the WavRangeReduction function to allow it to have the same input for saving algorithms (A dictionary of save algorithms) as the BatchReduce function. This will give more flexibility to scientist using the CLI to save without having to create a batch file, as BatchReduce basically prepares the data to be sent to WavRangeReduction.
To do this change, I have refactored the set_save function, which technically can be used as a command too, to process the dictionary of save algorithms.

Fixes #38502.

Further detail of work

There are two things to notice that can be further discussed in the review:

  • At some point in the preparation, I changed the dictionary argument with input save algorithms to a list of save algorithms, as I couldn't see that the extension value of the dictionary was being used anywhere. I decided to revert back those changes just in case some scientist are using it that way (as you can already save using BatchReduce) and this would their scripts.
  • I have changed somewhat the documentation page (https://docs.mantidproject.org/nightly/techniques/ScriptingSANSReductions.html) to try to make it a little bit easier to navigate, additionally updating the new call signature for WavRangeReduction and changing the example test to use the new interface with data from the LOQ demo in the Training Course Set data. This also uses an example Mask file with .toml extension , which I think is the currently supported format in the SANS group in ISIS.
  • I have reworded the deprecated warning if trying to use the old interface.

To test:

  • Build the docs.
  • Check the scripting page on the built docs> ../ScriptingSANSReductions.html, everything should be well formatted and typo free.
  • Run the example on the scripting page on Mantid. It should save the LOQDemo reduced files as intended.
  • Check code.

Added 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.

@adriazalvarez adriazalvarez added SANS Issues and pull requests related to SANS ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS labels Jan 17, 2025
@adriazalvarez adriazalvarez added this to the Release 6.13 milestone Jan 17, 2025
@adriazalvarez adriazalvarez marked this pull request as ready for review January 20, 2025 08:54
@MialLewis MialLewis self-assigned this Jan 20, 2025
Copy link
Contributor

@MialLewis MialLewis left a comment

Choose a reason for hiding this comment

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

Thanks for this, tested and working well.

Two very minor comments.

save_algs = []
if save_algorithms:
for key in save_algorithms.keys():
if key == "SaveRKH":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you consider updating this with structural pattern matching here?

I agree with your decision to roll back from a list to the pre-existing dictionary, it's probably not worth breaking existing scripts for the minor code quality upgrade!

"""
Mainly internally used by BatchMode. Provides the save settings.
Mainly used internally by BatchReduce and WavRangeReduction. Prepares the save state on the director
Copy link
Contributor

Choose a reason for hiding this comment

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

You've removed it from BatchReduce (a good improvement), though I appreciate WavRangeReduction is called in its place?

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 SANS Issues and pull requests related to SANS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polarized SANS metadata: test saving via the SANS command interface
2 participants