ISIS Reflectometry (Polref) interface: Implemented uniform time slicing options in 'Event Handling' tab #19014

Merged
merged 39 commits into from Mar 31, 2017

Conversation

@PranavBahuguna
Contributor

PranavBahuguna commented Feb 28, 2017

This PR adds two new slicing options to the Event Handling tab in the interface called'UniformEven and Uniform respectively. A third option LogValue has been added, but is currently non-functional and will be implemented in another issue.

To test:

  • If using windows, this must be tested in Release mode.
  • The interface may be accessed via Interfaces->Reflectometry->ISIS Reflectometry (Polref). From there, go to the Event Handling tab.
  • Each option can be separately activated by clicking on its radio button. Ensure that only one option is activated at any time.
  • Go to the Runs tab and load INTER_NR_test2.tbl (from the sample INTER dataset) and click on Process. This should reduce the table and produce several workspaces. Ensure that these are the same as committing the same process on master.
  • In Event Handling, select Custom slicing and enter values 0, 10, 20, 30. Go back to Runs, select the first group and process. This should produce sliced workspaces with time ranges of 0-10, 10-20 and 20-30 seconds respectively. Ensure that the results match that of master (although the workspaces will be named by slice index, rather than time range).
  • Now select Uniform even slicing and enter 5. Go back to Runs, select the first group and process. This should produce 5 evenly-sized sliced workspaces of each type.
  • Finally select Uniform slicing and enter 10. Runs 13460 and 13460 have a duration of 52 and 21 seconds respectively. Therefore this should produce 6 and 3 sliced workspaces of each type for each run.
    As only common slices are used for the stitched IvsQ workspaces, there should only be three sliced workspaces for them.
  • Open a new table and add run 38415 in the first row, with 38393 as transmission run. Process with each slicing option and ensure that the plot rows and plot group buttons work correctly and produce the desired result.
  • Repeat previous step (using one of the slicing options) and delete TOF_38415 from the workspaces dock. Ensure that when attempting to plot, the interface gives an error message.
  • Ensure documentation is accurate and up to date.

Fixes #18500.

Release notes have been updated


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? Is it well structured with small focussed classes/methods/functions?
  • Are there unit/system tests in place? Are the unit tests small and test the a 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?

  • How do the changes handle unexpected situations, e.g. bad input?

  • Has the relevant documentation been added/updated?

  • Is user-facing documentation written in a user-friendly manner?

  • Has developer documentation been updated if required?

  • Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

PranavBahuguna added some commits Feb 28, 2017

@raquelalvarezbanos raquelalvarezbanos self-assigned this Feb 28, 2017

PranavBahuguna added some commits Feb 28, 2017

+ else if (m_sliceType == m_slicingTypes[2])
+ values = m_ui.customEdit->text().toStdString();
+ else if (m_sliceType == m_slicingTypes[3])
+ values = m_ui.logValueComboBox->currentText().toStdString();

This comment has been minimized.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

Just a very minor point: I think this would be easier to read if we replaced m_slicingTypes[i] with the actual values we are using for the comparisons. I had to go back to QtReflEventView.h to check how the vector was initialized. Is there any reason not to do this?

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

Just a very minor point: I think this would be easier to read if we replaced m_slicingTypes[i] with the actual values we are using for the comparisons. I had to go back to QtReflEventView.h to check how the vector was initialized. Is there any reason not to do this?

This comment has been minimized.

@PranavBahuguna

PranavBahuguna Mar 7, 2017

Contributor

It was mostly just a little more convenient, but I see that it makes it less readable so I will change it.

@PranavBahuguna

PranavBahuguna Mar 7, 2017

Contributor

It was mostly just a little more convenient, but I see that it makes it less readable so I will change it.

+ </widget>
+ </item>
+ <item>
+ <spacer name="logValueSpacer">

This comment has been minimized.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

If slicing by log value is not implemented yet, can we remove the related widgets, code and documentation? I am concerned that scientist may get confused and think that this is fully functional (even if you have mentioned in the documentation that it is not implemented yet). The reason why I request removing this is because we had a meeting with scientists last week and their priorities have changed for this release, which means that slicing by log value may not get done for 3.10.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

If slicing by log value is not implemented yet, can we remove the related widgets, code and documentation? I am concerned that scientist may get confused and think that this is fully functional (even if you have mentioned in the documentation that it is not implemented yet). The reason why I request removing this is because we had a meeting with scientists last week and their priorities have changed for this release, which means that slicing by log value may not get done for 3.10.

+ auto data = row.second; // Vector containing data for this row
+ std::string runNo = row.second.at(0); // The run number
+
+ if (timeSlicingType == "UniformEven" || timeSlicingType == "Uniform") {

This comment has been minimized.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

Another very minor detail: if we do if (timeSlicingType != "Custom") instead we can avoid one check if timeSlicingType is Uniform (assuming we remove the currently unused LogValue option).

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

Another very minor detail: if we do if (timeSlicingType != "Custom") instead we can avoid one check if timeSlicingType is Uniform (assuming we remove the currently unused LogValue option).

+ and ending at ``300`` seconds.
+- **Log Value** - Not implemented yet, selecting this does not do anything for now.
+
+Workspaces will be named according to the index of the slice, e.g ``IvsQ_13460_slice_0``, ``IvsQ_13460_slice_1``, etc.

This comment has been minimized.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

Perfect, I think this is a very good explanation of what the tab is doing. Can we just add that slicing options are exclusive?

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

Perfect, I think this is a very good explanation of what the tab is doing. Can we just add that slicing options are exclusive?

GroupData groupNew;
std::vector<std::string> data;
for (const auto &row : group) {
data = row.second;
- data[0] = row.second[0] + "_" + std::to_string((int)startTimes[i]) +
- "_" + std::to_string((int)stopTimes[i]);
+ data[0] = row.second[0] + "_slice_" + std::to_string(i);

This comment has been minimized.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

This means that the way in which Custom slices are named has changed. I think it should be OK, but given that it will be different in the next release, could you add a sentence about this in the release notes so we remember to mention this to scientists before the release?

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

This means that the way in which Custom slices are named has changed. I think it should be OK, but given that it will be different in the next release, could you add a sentence about this in the release notes so we remember to mention this to scientists before the release?

+ m_mainPresenter->giveUserCritical("Workspace to slice not found: " + wsName,
+ "Time slicing error");
+ return;
+ }

This comment has been minimized.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

What if wsName exists in the ADS but it is not an event workspace? Should we check for if (mws)?

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

What if wsName exists in the ADS but it is not an event workspace? Should we check for if (mws)?

+ } else if (timeSlicingType == "Custom") {
+ parseCustom(timeSlicingValues, startTimes, stopTimes);
+ }
+

This comment has been minimized.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

I know you were just following the existing code, but I am wondering if we need to parseUniform and parseCustom everytime we want to plot a row or a group. Could we define the startTimes and stopTimes as member variables which are initialized in processGroupAsEventWS and then just use them in plotRow() and plotGroup()? That way we wouldn't need to check here again the type of slicing and calculate the startTimes and stopTimes again.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

I know you were just following the existing code, but I am wondering if we need to parseUniform and parseCustom everytime we want to plot a row or a group. Could we define the startTimes and stopTimes as member variables which are initialized in processGroupAsEventWS and then just use them in plotRow() and plotGroup()? That way we wouldn't need to check here again the type of slicing and calculate the startTimes and stopTimes again.

This comment has been minimized.

@PranavBahuguna

PranavBahuguna Mar 8, 2017

Contributor

After some investigation, I think we only need to store how many slices are used for each run in each group. We don't actually need the start/stop times after slicing is done.

@PranavBahuguna

PranavBahuguna Mar 8, 2017

Contributor

After some investigation, I think we only need to store how many slices are used for each run in each group. We don't actually need the start/stop times after slicing is done.

+
+ EXPECT_CALL(mockView, getTimeSlicingType())
+ .Times(Exactly(1))
+ .WillOnce(Return("UniformEven"));

This comment has been minimized.

@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

Just for information, if the value returned by the mocked view is not going to be used, you can just do:

EXPECT_CALL(mockView, getTimeSlicingType()).Times(Exactly(1));
@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

Just for information, if the value returned by the mocked view is not going to be used, you can just do:

EXPECT_CALL(mockView, getTimeSlicingType()).Times(Exactly(1));
@raquelalvarezbanos

This comment has been minimized.

Show comment
Hide comment
@raquelalvarezbanos

raquelalvarezbanos Mar 6, 2017

Contributor

Something is not right in terms of functionality. The following two cases should give the same results (testing with run INTER38415 and transmission INTER38393):

  • Custom slicing with Python list 0, 122, 244, 366, 488, 610
  • Uniform slicing with 5 even time slices

This is because the run duration (according to sample logs) is 610 seconds. The problem seems to be here, when debugging, I can see that totalDurationSec = 6, are we sure that getFirstPulseTime() and getLastPulseTime() are the methods we want?

Edit: the following:

  const auto run = mws->run();
  const auto totalDuration = run.endTime() - run.startTime();
  double totalDurationSec = totalDuration.total_seconds();

seems to do what we need.

Contributor

raquelalvarezbanos commented Mar 6, 2017

Something is not right in terms of functionality. The following two cases should give the same results (testing with run INTER38415 and transmission INTER38393):

  • Custom slicing with Python list 0, 122, 244, 366, 488, 610
  • Uniform slicing with 5 even time slices

This is because the run duration (according to sample logs) is 610 seconds. The problem seems to be here, when debugging, I can see that totalDurationSec = 6, are we sure that getFirstPulseTime() and getLastPulseTime() are the methods we want?

Edit: the following:

  const auto run = mws->run();
  const auto totalDuration = run.endTime() - run.startTime();
  double totalDurationSec = totalDuration.total_seconds();

seems to do what we need.

@raquelalvarezbanos

This comment has been minimized.

Show comment
Hide comment
@raquelalvarezbanos

raquelalvarezbanos Mar 15, 2017

Contributor

When I try to reduce run 38415 (transmission 38393) with a custom python list 0, 122, 244, 366, 488, 610, I get an error:

Logic Error in execution of algorithm FilterByTime
You need to specify either the StartTime or StopTime parameters; or both the AbsoluteStartTime and AbsoluteStopTime parameters; but not other combinations.

I think we should be able to reduce this run with these parameters. I don't remember seeing this error message the first time I tested this PR.

Contributor

raquelalvarezbanos commented Mar 15, 2017

When I try to reduce run 38415 (transmission 38393) with a custom python list 0, 122, 244, 366, 488, 610, I get an error:

Logic Error in execution of algorithm FilterByTime
You need to specify either the StartTime or StopTime parameters; or both the AbsoluteStartTime and AbsoluteStopTime parameters; but not other combinations.

I think we should be able to reduce this run with these parameters. I don't remember seeing this error message the first time I tested this PR.

@raquelalvarezbanos

This comment has been minimized.

Show comment
Hide comment
@raquelalvarezbanos

raquelalvarezbanos Mar 16, 2017

Contributor

This line is wrong, it should be

double totalDurationSec = totalDuration.total_seconds();

as suggested in a comment above. Having just seconds() does not produce the correct results.

In general, when working on a new piece of work/fixing something you should think about different ways of checking that results are correct. In this case, in addition to what you describe in this PR, you could use run INTER38415 (and transmission INTER38393) and check that the following three cases give the same result workspaces:

  1. Custom slicing with Python list 0, 122, 244, 366, 488, 610
  2. Uniform slicing with 5 even time slices
  3. Uniform slicing with 122 seconds

The reason is that run INTER38415 has a total duration of 610 seconds (which is something you can see from the logs), and 610 is divisible by 5, which means that the above cases should produce the same time slices and therefore the same output workspaces. Check this when you change that line and you'll see that, for instance, the IvsQ workspaces match exactly. If they don't, there's something wrong in functionality. If they do, functionality is likely to be correct (it will have to be tested in more detail and using other datasets by scientists), as different ways of specifying the same time slices produce the same results.

Contributor

raquelalvarezbanos commented Mar 16, 2017

This line is wrong, it should be

double totalDurationSec = totalDuration.total_seconds();

as suggested in a comment above. Having just seconds() does not produce the correct results.

In general, when working on a new piece of work/fixing something you should think about different ways of checking that results are correct. In this case, in addition to what you describe in this PR, you could use run INTER38415 (and transmission INTER38393) and check that the following three cases give the same result workspaces:

  1. Custom slicing with Python list 0, 122, 244, 366, 488, 610
  2. Uniform slicing with 5 even time slices
  3. Uniform slicing with 122 seconds

The reason is that run INTER38415 has a total duration of 610 seconds (which is something you can see from the logs), and 610 is divisible by 5, which means that the above cases should produce the same time slices and therefore the same output workspaces. Check this when you change that line and you'll see that, for instance, the IvsQ workspaces match exactly. If they don't, there's something wrong in functionality. If they do, functionality is likely to be correct (it will have to be tested in more detail and using other datasets by scientists), as different ways of specifying the same time slices produce the same results.

@raquelalvarezbanos

This comment has been minimized.

Show comment
Hide comment
@raquelalvarezbanos

raquelalvarezbanos Mar 20, 2017

Contributor

Results look good now.

:shipit:

Contributor

raquelalvarezbanos commented Mar 20, 2017

Results look good now.

:shipit:

@NickDraper NickDraper merged commit 2374776 into master Mar 31, 2017

9 checks passed

ClangFormat Jenkins build pull_requests-clang-format 12474 has succeeded
Details
Doxygen Jenkins build pull_requests-doxygen 11796 has succeeded
Details
Flake8 Jenkins build pull_requests-flake8 3120 has succeeded
Details
OSX Jenkins build pull_requests-osx 12996 has succeeded
Details
RHEL7 + System Tests Jenkins build pull_requests-rhel7 12866 has succeeded
Details
Ubuntu + Doc Tests Jenkins build pull_requests-ubuntu 13466 has succeeded
Details
Ubuntu Python 3 Jenkins build pull_requests-ubuntu-python3 1004 has succeeded
Details
Windows Jenkins build pull_requests-win7 13730 has succeeded
Details
cppcheck Jenkins build pull_requests-cppcheck 13456 has succeeded
Details

@NickDraper NickDraper deleted the 18500_RRO_implement_other_tslice_options branch Mar 31, 2017

@gemmaguest gemmaguest moved this from Open PR to Done in ISIS Large Scale Structures Apr 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment