Make listener properties in StartLiveData dynamic #17926

MikeHart85 · 2016-11-01T17:50:25Z

As required by More Flexible LiveListener design document.

This effectively reverts #234, plus modifications and additions to still achieve its purpose.

SpectraList and PeriodList properties are no longer hard-coded as part of StartLiveData.

Instead, StartLiveData will dynamically add the properties of the specific LiveListener it uses once the Instrument is set.

The Python simpleapi has been updated with a specialized version of StartLiveData to allow evaluating other arguments based on the value of the Instrument argument.

To test:

See associated issue for testing GUI behaviour.

For Python interface:

Run a FakeISISHistoDAE algorithm with NPeriods=5, NSpectra=10 and NBins=100.
Run the following code in the console:

ConfigService.setFacility('TEST_LIVE')
StartLiveData(Instrument='ISIS_Histogram', OutputWorkspace='OutWS', AccumulationWorkspace='AccWS', UpdateEvery=1, AccumulationMethod='Replace', PeriodList=[1,3], SpectraList=[2,4,6])

This should work both before and after this change.

Fixes #17912.

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.

OwenArnold · 2016-11-02T15:08:58Z

Framework/PythonInterface/mantid/simpleapi.py

    lhs = _kernel.funcinspect.lhs_info()
+    if lhs[0] > 0:  # Number of terms on the lhs
+        raise RuntimeError("Assigning the output of StartLiveData is currently "


OK is this the fix until we figure out how to deal with optional output workspaces?

After talking to @martyngigg, we decided that for now this will have to be considered a limitation of the simpleapi. There are several issues with capturing the output:

Unwrapping is not working properly because of the optional "AccumulationWorkspace" output, and there's no easy fix for this.

Even capturing in a single variable (so capturing output as a tuple without unwrapping) is setting the AccumulationWorkspace argument to the name of the variable on the left.

There if the name of the first variable on the left matches the value of the "OutputWorkspace" argument, there is very inconsistent / buggy behaviour because now both OutputWorkspace and AccumulationWorkspace have the same name.

Since OutputWorkspace is being periodically replaced in the ADS, any reference to it you get in Python is invalidated shortly after you get it.

OwenArnold · 2016-11-02T15:10:43Z

Framework/LiveData/src/StartLiveData.cpp

@@ -85,6 +70,38 @@ void StartLiveData::init() {
                  "completes.");
 }

+/**
+ * After Instrument property is set copy any properties that the instrument's


If i'm being picky: I'd say that this comment will not necessarily apply to future modifications since the listener will not be tied to the instrument.

Agreed, I thought about that as well. But that will be a separate pull request, and so for now this comment reflects the behaviour implemented here.

The code here will need to be modified again once we make that change, since the propName == "Instrument" condition will no longer be correct. Need to make sure to update this comment at that point.

Sounds fine. Make a comment somewhere so that you don't forget. But you're right we shouldn't add comments to match future modifications.

OwenArnold · 2016-11-02T15:13:08Z

Framework/LiveData/src/StartLiveData.cpp

+    }
+
+    // Add new listener's properties
+    auto listener = LiveListenerFactory::Instance().create(


Might be useful to have a comment about this (following) code relating to the transfer of ownership. Or to put this in a separate named method.

Addressed by refactoring with two helper functions and simplifying code a bit.

OwenArnold · 2016-11-02T15:27:53Z

docs/source/release/v3.9.0/framework.rst

@@ -40,6 +41,7 @@ Python Algorithms

 - :ref:`MatchPeaks <algm-MatchPeaks>` performs circular shift operation (numpy roll) along the x-axis to align the peaks in the spectra.
 - :ref:`FindEPP <algm-FindEPP>` is improved to better determine the initial parameters and range for the fitting.
+- :ref:`StartLiveData <algm-StartLiveData>` can now accept LiveListener properties as parameters, based on the Instrument parameter.


I would be careful about "Instrument parameter". I initially misread this as "Instrument parameters" which is a completely different thing.

I've tried clarifying the wording a bit, let me know if it works better this way.

OwenArnold · 2016-11-02T15:31:14Z

docs/source/release/v3.9.0/framework.rst

@@ -40,6 +41,7 @@ Python Algorithms

 - :ref:`MatchPeaks <algm-MatchPeaks>` performs circular shift operation (numpy roll) along the x-axis to align the peaks in the spectra.
 - :ref:`FindEPP <algm-FindEPP>` is improved to better determine the initial parameters and range for the fitting.
+- :ref:`StartLiveData <algm-StartLiveData>` can now accept LiveListener properties as parameters, based on the Instrument parameter.


I see that there is no concepts page for the LiveListener. We should definitely add that as part of a separate PR. However, I note that the algorithm rst for StartLiveData has not been updated as part of this PR. I think that it would be important to have some information in there about the fact that the algorithm is dynamically going to be picking up properties from it's listener such as SpectrumList and PeriodList otherwise that information will just have vanished, and I believe that these changes were originally made (the ones you just reverted), because users complained about not seeing documentation for them.

Addressed by adding a section on listener properties to StartLiveData rst.

OwenArnold · 2016-11-03T14:14:53Z

docs/source/algorithms/StartLiveData-v1.rst

+PeriodList and SpectraList are properties of the ISISHistoDataListener. They
+are available as arguments in this call because Instrument is set to
+'ISIS_Histogram', which uses that listener.
+


Good documentation!

Thank you! :)

OwenArnold · 2016-11-07T09:47:38Z

Michael Hart added 2 commits November 1, 2016 17:33

Refs #17912 Dynamically add listener properties

cc0b2f1

Refs #17912 Specialize Python interface for StartLiveData

6bcdb11

MikeHart85 added Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) State: In Progress labels Nov 1, 2016

MikeHart85 added this to the Release 3.9 milestone Nov 1, 2016

MikeHart85 assigned OwenArnold Nov 1, 2016

Michael Hart added 2 commits November 2, 2016 07:56

Refs #17912 clang format

afb3468

Refs #17912 Updated release notes

72135ec

MikeHart85 added State: In Progress and removed State: In Progress labels Nov 2, 2016

Michael Hart added 2 commits November 2, 2016 11:24

Refs #17912 Python argument default should not be list

fd15f87

Refs #17912 Disallow assigning output of StartLiveData in Python

1298481

MikeHart85 removed the State: In Progress label Nov 2, 2016

OwenArnold reviewed Nov 2, 2016

View reviewed changes

MikeHart85 added the State: In Progress label Nov 3, 2016

Michael Hart added 4 commits November 3, 2016 13:43

Refs #17912 Clarified release note to avoid misunderstanding

3b01bcc

Refs #17912 Simplified and refactored copying of properties

2146976

Refs #17912 Added explanation of listener properties to docs

3cf11a7

Refs #17912 clang format

ecd44eb

OwenArnold reviewed Nov 3, 2016

View reviewed changes

MikeHart85 removed the State: In Progress label Nov 3, 2016

AndreiSavici merged commit 865f001 into master Nov 7, 2016

AndreiSavici deleted the 17912_Dynamic_Listener_Properties branch November 7, 2016 21:57

MikeHart85 mentioned this pull request Sep 15, 2017

StartLiveData sometimes ignores Listener Properties #20489

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make listener properties in StartLiveData dynamic #17926

Make listener properties in StartLiveData dynamic #17926

MikeHart85 commented Nov 1, 2016 •

edited

OwenArnold Nov 2, 2016

MikeHart85 Nov 3, 2016

OwenArnold Nov 2, 2016

MikeHart85 Nov 3, 2016

OwenArnold Nov 3, 2016

OwenArnold Nov 2, 2016

MikeHart85 Nov 3, 2016

OwenArnold Nov 2, 2016

MikeHart85 Nov 3, 2016

OwenArnold Nov 2, 2016

MikeHart85 Nov 3, 2016

OwenArnold Nov 3, 2016

MikeHart85 Nov 3, 2016

OwenArnold commented Nov 7, 2016

Make listener properties in StartLiveData dynamic #17926

Make listener properties in StartLiveData dynamic #17926

Conversation

MikeHart85 commented Nov 1, 2016 • edited

Reviewer

Code Review

Functional Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenArnold commented Nov 7, 2016

MikeHart85 commented Nov 1, 2016 •

edited