Added validation for event workspaces in ConjoinWorkspaces #20006

josephframsay · 2017-07-14T11:24:31Z

Validate EventWorkspaces according to their bins

To test:

ws = CreateSampleWorkspace("Event")
ws2 = CreateSampleWorkspace("Event")

ws_cropped = CropWorkspace(ws, StartWorkspaceIndex=0, EndWorkspaceIndex=2)
ws2_cropped = CropWorkspace(ws2, StartWorkspaceIndex=3, EndWorkspaceIndex=5)

ws2_cropped = Rebin(ws2_cropped, Params="100,200,700")

ConjoinWorkspaces(ws_cropped, ws2_cropped)

Running the above script in Mantid should produce a ValueError, where previously there was an unhandled exception.

Fixes #5564 .

Release Notes
Does not need to be in the release notes.

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.

KarlPalmen

I just want answers to my comments. The python script works as expected.

KarlPalmen · 2017-07-14T14:09:58Z

Framework/Algorithms/src/ConjoinWorkspaces.cpp

@@ -55,6 +55,8 @@ void ConjoinWorkspaces::exec() {
  }

  if (event_ws1 && event_ws2) {
+    this->validateInputs(*event_ws1, *event_ws2);
+
    // We do not need to check that binning is compatible, just that there is no
    // overlap
    // make sure we should bother checking


Is this comment still correct?

KarlPalmen · 2017-07-14T14:13:16Z

Framework/Algorithms/src/ConjoinWorkspaces.cpp

-  // Delete the second input workspace from the ADS
-  AnalysisDataService::Instance().remove(getPropertyValue("InputWorkspace2"));
-  // Set the result workspace to the first input
-  setProperty("InputWorkspace1", output);
 }


Have you done anything besides replace (if { code, return } more_code) with if {code} else {more_code} ?

I see you added this->validateInputs(*event_ws1, *event_ws2);

KarlPalmen

I haven't put names to all the new faces yet. So if you want to talk to me about this, please do so.

KarlPalmen

I'm happy with this.

martyngigg · 2017-07-18T08:47:26Z

The RHEL7 system test has picked up a failure in LETReduction. It is now hitting this validation.

martyngigg · 2017-07-18T08:49:42Z

It was explicitly stated in the code that the binning doesn't matter for events so I think we need to skip matching those and just test everything else.

…ct/mantid into 5564_validate_inputs_conjoin

martyngigg · 2017-07-19T09:55:59Z

Framework/Algorithms/src/WorkspaceJoiners.cpp

-        "Both input workspaces must have common binning for all their spectra");
-    throw std::invalid_argument(
-        "Both input workspaces must have common binning for all their spectra");
+  // Workspaces with point data are allowed to have different binning


I'm not this is exactly what we want here. Point data and event workspaces are different things. EventWorkspace is a special type of MatrixWorkspace that stores collections of signal/timestamp pairs per spectrum. Histograms of these collections are then generated on the fly from a current set of bin boundaries. Here the check is unimportant because of this on-the-fly generation.

For the MatrixWorkspace case, regarless of point or histogram status, the algorithm appends the spectra from the rhs to the lfs workspace but uses the X values exclusively from the lhs. If they are not the same in both cases then the numbers at the end could be misleading.

Perhaps the easiest way to solve this is to add a flag to validateInputs to disable the bin checking and then set this to true on line 58 of ConjoinWorkspaces.cpp as we already have checked for event vs non-event here?

martyngigg

Good, we're almost there. I think there is just one small tidy up remaining.

martyngigg · 2017-07-19T12:25:36Z

Framework/Algorithms/inc/MantidAlgorithms/WorkspaceJoiners.h

@@ -56,6 +56,8 @@ class DLLExport WorkspaceJoiners : public API::Algorithm {

  using Mantid::API::Algorithm::validateInputs;
  void validateInputs(const API::MatrixWorkspace &ws1,


We can collapse these into a single method with a default parameter:

void validateInputs(const API::MatrixWorkspace &ws1, const API::MatrixWorkspace &ws2, const bool checkBinning=true);

rather than needing two methods.

martyngigg

Thanks, @josephframsay. This looks good now.

Refs #5564 Added validation for event workspaces

1eac766

josephframsay added Framework Issues and pull requests related to components in the Framework Misc: Easy labels Jul 14, 2017

josephframsay added this to the Release 3.11 milestone Jul 14, 2017

Refs #5564 clang format changes applied

cfe264a

KarlPalmen self-assigned this Jul 14, 2017

KarlPalmen self-requested a review July 14, 2017 12:52

KarlPalmen reviewed Jul 14, 2017

View reviewed changes

Re #5564 Changed misleading comment in ConjoinWorkspaces

387041d

KarlPalmen approved these changes Jul 17, 2017

View reviewed changes

josephframsay mentioned this pull request Jul 17, 2017

ConjoinWorkspaces add option to set Y axis units and label #20017

Merged

7 tasks

josephframsay added the State: In Progress label Jul 18, 2017

josephframsay added 4 commits July 18, 2017 10:20

Re #5564 Update system test to rebin before conjoining

3ffb0d2

Re #5564 Allow EventWSs to have different binning in WorkspaceJoiners

b405f1b

Merge branch '5564_validate_inputs_conjoin' of github.com:mantidproje…

58b1905

…ct/mantid into 5564_validate_inputs_conjoin

Re #5564 Applied clang-format patch

5ee2168

josephframsay removed the State: In Progress label Jul 19, 2017

martyngigg self-assigned this Jul 19, 2017

martyngigg reviewed Jul 19, 2017

View reviewed changes

josephframsay added 3 commits July 19, 2017 12:21

Re #5564 Added flag to ValidateInputs to disable binning check

aced5b9

Re #5564 Applied clang-format patch

e4333dc

Re #5564 Applied clang-format patch

57b8369

martyngigg requested changes Jul 19, 2017

View reviewed changes

josephframsay added 2 commits July 19, 2017 14:12

Re #5564 Combined the two validateInputs method with default parameter

e453268

Re #5564 Applied clang-format patch

eb4864d

peterfpeterson self-requested a review July 19, 2017 14:43

martyngigg approved these changes Jul 20, 2017

View reviewed changes

AndreiSavici merged commit 716cd9f into master Jul 24, 2017

AndreiSavici deleted the 5564_validate_inputs_conjoin branch July 24, 2017 18:28

DanielMurphy22 added Induction Straightforward, low risk issues suitable for new starters and removed Induction 1 labels Nov 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added validation for event workspaces in ConjoinWorkspaces #20006

Added validation for event workspaces in ConjoinWorkspaces #20006

josephframsay commented Jul 14, 2017

KarlPalmen left a comment

KarlPalmen Jul 14, 2017

KarlPalmen Jul 14, 2017

KarlPalmen Jul 14, 2017

KarlPalmen left a comment

KarlPalmen left a comment

martyngigg commented Jul 18, 2017

martyngigg commented Jul 18, 2017

martyngigg Jul 19, 2017

martyngigg left a comment

martyngigg Jul 19, 2017

martyngigg left a comment

		@@ -56,6 +56,8 @@ class DLLExport WorkspaceJoiners : public API::Algorithm {

		using Mantid::API::Algorithm::validateInputs;
		void validateInputs(const API::MatrixWorkspace &ws1,

Added validation for event workspaces in ConjoinWorkspaces #20006

Added validation for event workspaces in ConjoinWorkspaces #20006

Conversation

josephframsay commented Jul 14, 2017

Reviewer

Code Review

Functional Tests

KarlPalmen left a comment

Choose a reason for hiding this comment

KarlPalmen Jul 14, 2017

Choose a reason for hiding this comment

KarlPalmen Jul 14, 2017

Choose a reason for hiding this comment

KarlPalmen Jul 14, 2017

Choose a reason for hiding this comment

KarlPalmen left a comment

Choose a reason for hiding this comment

KarlPalmen left a comment

Choose a reason for hiding this comment

martyngigg commented Jul 18, 2017

martyngigg commented Jul 18, 2017

martyngigg Jul 19, 2017

Choose a reason for hiding this comment

martyngigg left a comment

Choose a reason for hiding this comment

martyngigg Jul 19, 2017

Choose a reason for hiding this comment

martyngigg left a comment

Choose a reason for hiding this comment