941 append scaling to ws name #951

GuiMacielPereira · 2023-10-10T15:31:40Z

Summary

Created new class that returns name of workspaces for the operations of add, subtract, rebose and scale
Updated tests with new class, so any changes to naming conventions will not have tests failing

Description of work:

The main observable change is the new format of naming workspaces after subtraction
Instead of the suffix _subtracted, I changed it to include the scaling factor of the background workspace to _minus_ssf_0.95, as seen in:

Also altered the scaling suffix to match new naming convention

Question

I altered the tests so that the new class I introduced gets imported into the test and the correct name of the workspace is always used.
This means that if this class gets changed to include a different naming convention, the tests will continue to pass.
Is this good practice or should I hard-code the workspace name string into the tests?

To test:

In the workspace manager, load a workspace and try the arithmetic operations:
- Scale
- Subtract
- Rebose
- Sum
Give your opinion on whether the naming convention I implemented makes sense
Look at the new class WorkspaceNameAppender and tell me if it's fine (maybe needs an initializer method?)
Look at the location of the new class (under workspaces.helperfunctions), is it the best location or should I move somewhere else?

Fixes #941 .

The new class WorkspaceNameAppender returns the changed workspace name for 4 operations: Sum, Subtract, Scale and Rebose. This implementation makes it easier to change the naming conventions if needed. Altered tests to import this class, so that tests get updated if naming convention changes.

MialLewis · 2023-11-02T15:47:49Z

src/mslice/models/workspacemanager/workspace_algorithms.py

@@ -25,6 +25,7 @@
 from mslice.workspace.pixel_workspace import PixelWorkspace
 from mslice.workspace.histogram_workspace import HistogramWorkspace
 from mslice.workspace.workspace import Workspace
+from mslice.workspace.helperfunctions import WorkspaceNameAppender


Could we rename this WorkspaceNameHandler or something, as we will have to prepend, not just append?

MialLewis

This looks good. I can think of a few other instances where this would be useful to use, could you apply them here:
https://github.com/mantidproject/mslice/blob/5af891e298559faef096b834992c4cafc00d8bd4/src/mslice/util/mantid/algorithm_wrapper.py#L55&#L59

Also, we could do with getting rid of checks like this

mslice/src/mslice/workspace/helperfunctions.py

Line 88 in 5af891e

    
           if hasattr(workspace, str(ws)) and ws is not None and ws.name().endswith('_HIDDEN'):

Something like WorkspaceNameHander.isHidden(wsName).

If you search for __MSL __HIDDEN etc. will you will find other instances too.

My ambition is that we can look at this one class to understand what has happened to a workspace, rather than traipsing through the code looking for name modifications.

- Altered class for ws name handling of ws operatiosn - Started looking into other sections of code where names of ws are altered - Changed some parts of algorithm wrapper and scripting files - Currently it's a mess because I don't understand what these routines are doing yet

- Named methods in WorkspaceNameHandler according to their function - replaced several parts of code with new methods

- Removed '.' from workspaces names because might cause trouble with mantid - Added two methods for appending sufixes to workspaces

- Checked that changing the suffixes in my code makes tests fail - The prefixes '__MSL', '__MSLTMP' and suffix '_HIDDEN' are not covered by the unit tests

GuiMacielPereira · 2023-11-10T17:47:50Z

Put all workspace name changes into the same class
The workspace names should not have the character '.' and so I replaced it by '_'
- (If using the character '.' and produce a script, the script will include a RenameWorkspace algorithm and change the name of the workspace to something weird)
The prefixes __MSL and __MSLTMP and suffix _HIDDEN are not covered by the tests, so I did a visual test:
- Followed the following procedure:
  - Load ws MAR21335_Ei60meV
  - Subtract this workspace with itself with some scaling factor
  - Select the subtracted workspace and plot a Slice plot (using default parameters)
- Check names of workspace in Mantid:
  - These are the same workspaces that appeared before making changes to the code
  - __MSL and _HIDDEN are correctly added to ws names
The remaining prefixes and suffixes are covered by the tests
- I checked that altering one of these in the code will cause some tests to fail
- My new implementation of previous names did not break old tests
My changes touched files that produce mslice scritps, so I also did a check that all is good with the scripting
- I followed the procedure described above for the Slice plot and clicked to produce a script
- The script matches with the script from before the changes (excepting workspace names that are meant to change)
- Oddly enough, I detected one very small difference that I can't quite understand:
  - The new script uses the Load() function with the argument for Filename as a normal string, whilst the original script before any changes passes this argument as a raw string. I don't think this makes any significant difference.
- Finally, I tried to run the script I produced on mantid, but this is not possible, as the mslice.cli module does not support the Minus() algorithm (But this is out of scope for this PR)

- Prevented scaling factor in workspace names from having more than 2 decimal places

for more information, see https://pre-commit.ci

MialLewis

Good work, have done an initial review of implementation. Let me know what you think.

MialLewis · 2023-11-17T16:13:43Z

src/mslice/workspace/helperfunctions.py

@@ -116,3 +115,60 @@ def __exit__(self, exc_type, exc_val, exc_tb):
        if self.workspace:
            self.workspace.remove_saved_attributes()
        return True
+
+
+class WorkspaceNameHandler:


I'm surprised there are so many options! I don't think it's ideal to have a separate function for each combination of functions. It's plausible in the future we have to add more, and therefore the number of combination functions will increase exponentially.

I wonder if it's a good idea to instead have a WorkspaceNameOptions class, that has a constructor that default sets everything to false via named arguments.

We could then do a combination of functions by doing:
WorkspaceNameHandler.set_workspace_options(workspace.name, WorkSpaceOptions(hidden_from_mslice=True, hidden_from_ADS=True))

You would then only need individual functions in the WorkspaceNameHandler class, and these would not have to be exposed.

Perhaps when checking we could then do:
WorkspaceNameHandler.assert_workspace_options(workspace, WorkspaceOptions(hidden_from_mslice=True, hidden_from_ADS=True)

I have now implemented these suggestions, but instead of creating and entirely separate WorkspaceOptions class, I preferred to opt for two methods get_name and assert_name that mimicks this. So in this case one would do WorkspaceNameHandler(workspace.name).get_name(hidden_from_mslice=True, hidden_from_ADS=True and WorkspaceNameHandler(workspace.name).assert_name(hidden_from_mslice=True, hiddden_from_ADS=True).

MialLewis · 2023-11-17T16:18:31Z

src/mslice/workspace/helperfunctions.py

+    def merged(self) -> str:
+        return self.add_sufix('_merged')
+
+    def isHiddenFromMsl(self) -> bool:


You've swapped from snake case to camel case here, worth having a flick through these if you haven't already: https://developer.mantidproject.org/Standards/PythonStandards.html

This reverts commit 9883cf5.

GuiMacielPereira · 2024-01-11T14:13:12Z

Altered class to follow Mial's suggestions.
As Mial is on his sabbatical, here is the full breakdown of the manual test to perform for someone unfamiliar with this PR:

Load MAR21335_Ei60meV into mslice
On the Slice interface, click Display
These hidden workspaces should appear on the mantid ADS:
Go to the plot window, and click to generate script to clipboard
Open a new tab on the mantid editor, paste the script and run it to check it produces the same plot
Now go back to the mslice interface and click to subtract the ws with itself by some scaling factor:
A new hidden workspace should appear on the mantid ADS with the suffix _HIDDEN
Try the other workspace arithmetic operations and check the new naming convention
Try deleting a workspace in mslice and check it is successfully deleted in the mantid ADS

GuiMacielPereira linked an issue Oct 10, 2023 that may be closed by this pull request

Prevent workspace overwriting when scaling during workspace arithmetic #941

Open

Removed commented out sections

29ded70

MialLewis self-assigned this Oct 17, 2023

MialLewis reviewed Nov 2, 2023

View reviewed changes

MialLewis requested changes Nov 2, 2023

View reviewed changes

GuiMacielPereira added 5 commits November 7, 2023 15:18

Changed methods to suitable variable names

0f687d3

- Named methods in WorkspaceNameHandler according to their function - replaced several parts of code with new methods

Removed artifact comment

a6b9d1b

Changed naming of scaling ws and added two more methods

5a703ca

- Removed '.' from workspaces names because might cause trouble with mantid - Added two methods for appending sufixes to workspaces

Updated unit tests

b9b4a0c

- Checked that changing the suffixes in my code makes tests fail - The prefixes '__MSL', '__MSLTMP' and suffix '_HIDDEN' are not covered by the unit tests

GuiMacielPereira added 3 commits November 13, 2023 10:37

Fixed ws names for scaling

37beb37

- Prevented scaling factor in workspace names from having more than 2 decimal places

Merge branch 'main' into 941_append_scaling_to_ws_name

633ea86

Fixed artifacts from merge

fda5254

GuiMacielPereira force-pushed the 941_append_scaling_to_ws_name branch from 24d3131 to fda5254 Compare November 13, 2023 16:16

Fixed long line and removed unused import

83ba1b2

GuiMacielPereira force-pushed the 941_append_scaling_to_ws_name branch from 325caeb to 83ba1b2 Compare November 13, 2023 16:19

pre-commit-ci bot and others added 2 commits November 13, 2023 16:20

[pre-commit.ci] auto fixes from pre-commit.com hooks

f2b2527

for more information, see https://pre-commit.ci

Fixed missing blank line

abdb380

GuiMacielPereira requested a review from MialLewis November 14, 2023 10:20

MialLewis requested changes Nov 17, 2023

View reviewed changes

GuiMacielPereira added 7 commits January 10, 2024 16:56

Updated class for clearer usability

4e214dc

Fixed Flake8 issues

9883cf5

Revert "Fixed Flake8 issues"

84d3fe7

This reverts commit 9883cf5.

Corrected fix for flake8 issues

dfe981a

Merge branch 'main' into 941_append_scaling_to_ws_name

9a68e1a

Updated a recent commit to use the new ws name format

f29879e

Removed outdated comment

2b12bdc

GuiMacielPereira requested a review from SilkeSchomann January 11, 2024 14:20

GuiMacielPereira assigned SilkeSchomann Jan 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

941 append scaling to ws name #951

941 append scaling to ws name #951

GuiMacielPereira commented Oct 10, 2023

MialLewis Nov 2, 2023 •

edited

Loading

MialLewis left a comment •

edited

Loading

GuiMacielPereira commented Nov 10, 2023

MialLewis left a comment

MialLewis Nov 17, 2023

GuiMacielPereira Jan 11, 2024

MialLewis Nov 17, 2023

GuiMacielPereira commented Jan 11, 2024

941 append scaling to ws name #951

Are you sure you want to change the base?

941 append scaling to ws name #951

Conversation

GuiMacielPereira commented Oct 10, 2023

MialLewis Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

MialLewis left a comment • edited Loading

Choose a reason for hiding this comment

GuiMacielPereira commented Nov 10, 2023

MialLewis left a comment

Choose a reason for hiding this comment

MialLewis Nov 17, 2023

Choose a reason for hiding this comment

GuiMacielPereira Jan 11, 2024

Choose a reason for hiding this comment

MialLewis Nov 17, 2023

Choose a reason for hiding this comment

GuiMacielPereira commented Jan 11, 2024

MialLewis Nov 2, 2023 •

edited

Loading

MialLewis left a comment •

edited

Loading