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

Change results group workspace suffix for single fits in QENS Fitting interface #39067

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

Conversation

adriazalvarez
Copy link
Contributor

@adriazalvarez adriazalvarez commented Mar 14, 2025

Description of work

Fixes #38827 .

This PR changes the suffix of the results output groups that are generated form the qens fitting interface after a fitting. When the fit involves more than one workspace (multifit), the results group has multiple workspaces, while when the fit contains only one single workspace (single fit) the results group has one workspace.
The group has the suffix _Results while the workspaces within the group have each suffix _Result.
This generates problems when mantid project is saved: The group workspaces containing one member are saved and reloaded as matrix workspaces instead of group workspaces, but with the name of the group (so that workspaces with groups ending in "_Result" will take the name of the group and end with "_Results" when loaded back). This is generally inconsequential for most Mantid workflows, but in the case of the Function Q tab of the QENS fitting interface, the load dialog seeks for a workspace or file ending with the suffix "_Result", and compatible files are not detected.
This is not really a bug in the Function Q interface, but rather a clash between two incompatible designs within Mantid.

  1. Is it not ideal that group workspaces with one member only are loaded as matrix workspaces, as it may override the intent of the user in some cases (as in function q interface). This is a consequence of the hierarchical structure of the nexus files, each workspace is saved as a NXgroup in the nexus files, so the way of the loader to distinguish groups is whether the root contains multiple NXgroups. Therefore is it not possible to distinguish whether something is a matrix workspace or a group of one member and ws are loaded as the former. One solution to this would be to alter SaveNexusProcessed algorithm and add a metadata field to the nexus field indicating whether something is being saved as a one workspace group, so that the loader can distinguish. This change is not very complicated but alters the savers/loaders in a more fundamental way and will likely require a separate investigation, as it may not be a good change when this is only affecting the funciton q interface.
  2. The QENS Fitting interface is composed of many intertwined classes and relies too much on cues like particular workspace suffices to load data. Adding small features or changing the way to retrieve data from ADS is constantly a source of bugs and errors (still to this day). In my opinion this interface would benefit from a deep overhaul, but as adding new features in spectroscopy is discontinued at the moment and would required long developing time, I have disregarded this.

Summary of work

What I have done in this PR is to choose a compromise solution, when the _Results group contains only one workspace member, the whole group will end with the suffix _Result instead. This will prompt the project saver to save the file with the correct suffix so when the workspace is loaded as a matrix workspace, it will be detected by the function q interface.

Additionally, Spencer made me aware of a bug that involved double plotting of some data from the output plot widget of the interfaces when the Plot button is clicked. As the solution for this was very simple, I have added it on this PR (and on release notes)

Fixes #38827.

Further detail of work

Additionally, tests are added. I have slightly refactored the test suite, and also added type hints to IndirectReplaceFit... algorithm.

To test:

  • Check everything is in order in manual testing for qens fitting: https://developer.mantidproject.org/Testing/Inelastic/QENSFittingTests.html
  • Note that the fit for single workspaces will create a group workspace ending in _Result.
  • When you have finished testing the convolution tab. Save Mantid project, then load Mantid project, when the project is loaded, the matrix workspace on the ADS should end in _Result and be detectable from the Function Q tab.

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 Indirect/Inelastic Issues and pull requests related to indirect or inelastic Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) labels Mar 17, 2025
@adriazalvarez adriazalvarez marked this pull request as ready for review March 18, 2025 07:38
@sf1919 sf1919 added this to the Release 6.13 milestone Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) Indirect/Inelastic Issues and pull requests related to indirect or inelastic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add workspace in Function(Q) QENS Fitting not finding correct files
3 participants