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

Sans renames workspace groups correctly #22615

Merged
merged 8 commits into from
Jul 2, 2018
Merged

Conversation

Matthew-Andrew
Copy link
Contributor

@Matthew-Andrew Matthew-Andrew commented Jun 19, 2018

Description of work.

This changes how the workspaces within workspace groups are renamed in the sans reduction. The individual members of the group are now renamed in line with the group itself.

Report to: sarah.rogers@stfc.ac.uk

To test:

The test files for this PR are available from //olympic/babylon5/Scratch/Matthew Andrew/SANS2DSAMPLE

  • Open the old Isis Sans GUI,
  • Select SANS2D as the instrument
  • Load the maskfile from the folder (this is the *.txt file)
  • Select Batch mode and load a batch file (this is the *.csv file)
  • Type 1:1:3 into the event slices box. This is specifying the event slices you want to reduce and in this case between 1 and 3 in steps of 1
  • Click 1D Reduce
  • At the end of the reduction confirm that there is a workspace group called line_1_reduction_rear which contains line_1_reduction_t1_T2_rear and line_1_reduction_t2_T3_rear.

Fixes #18933.

Release notes are here #22628

  • Yes
  • No

Reviewer

Please comment on the following (full description):

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 are changes in the release notes then do they describe the changes appropriately?
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.

@Matthew-Andrew Matthew-Andrew added Component: Python SANS Issues and pull requests related to SANS labels Jun 19, 2018
@Matthew-Andrew Matthew-Andrew added this to the Release 3.13 milestone Jun 19, 2018
@Matthew-Andrew Matthew-Andrew changed the title Sans now renames workspace groups Re #18933 Sans renames workspace groups correctly Jun 20, 2018
@MatthewBowles MatthewBowles self-assigned this Jun 28, 2018
Copy link
Member

@martyngigg martyngigg left a comment

Choose a reason for hiding this comment

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

Looking at the original issue the suggested naming scheme suggested as an example:

SS0P4LkinSemiShort_rear_1D_1.75_16.5_t0.00_T60.00

With the changes I get a group called line_1_reduction_rear and the sub members:

  • line_1_reduction_t1.00_T2.00_rear
  • line_1_reduction_t2.00_T3.00_rear

Should the _rear be after the line_1_reduction followed by the t1.... or have I misunderstood the intention?

@keeeto
Copy link
Contributor

keeeto commented Jun 28, 2018

This is crashing on OSX.

@martyngigg
Copy link
Member

As discussed offline this is a quick fix to improve the naming and we can get feedback on it. Not marking as passed until the OSX issue is addressed

@keeeto
Copy link
Contributor

keeeto commented Jun 28, 2018

The OSX issue is also present on master branch and older release versions of Mantid. Therefore it is not related to this PR. I have created a separate issue #22702.

@martyngigg
Copy link
Member

@Mantid-Matthew The GUI output looks good but it seems that some system tests are now failing

@martyngigg
Copy link
Member

@Mantid-Matthew There still seems to be some system tests failing I'm afraid.

Copy link
Member

@martyngigg martyngigg left a comment

Choose a reason for hiding this comment

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

This looks good now.

@ianbush ianbush self-assigned this Jul 2, 2018
@ianbush ianbush merged commit 8ce1af7 into master Jul 2, 2018
@ianbush ianbush deleted the sans_event_slice_group_names branch July 2, 2018 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SANS Issues and pull requests related to SANS
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Make name of WorkspaceGroup and child workspaces consistent for sliced reductions.
5 participants