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

Make listener properties in StartLiveData dynamic #17926

Merged
merged 10 commits into from
Nov 7, 2016
5 changes: 5 additions & 0 deletions Framework/LiveData/inc/MantidLiveData/StartLiveData.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ class DLLExport StartLiveData : public LiveDataAlgorithm {
private:
void init() override;
void exec() override;
void afterPropertySet(const std::string &) override;

void copyListenerProperties(
const boost::shared_ptr<Mantid::API::ILiveListener> &listener);
void removeListenerProperties();
};

} // namespace LiveData
Expand Down
61 changes: 46 additions & 15 deletions Framework/LiveData/src/StartLiveData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,21 +59,6 @@ void StartLiveData::init() {
"If you specify 0, MonitorLiveData will not launch and you will get only "
"one chunk.");

// Properties used with ISISHistoDataListener
declareProperty(make_unique<ArrayProperty<specnum_t>>("SpectraList"),
"An optional list of spectra to load. If blank, all "
"available spectra will be loaded. Applied to ISIS histogram"
" data only.");
getPointerToProperty("SpectraList")->setGroup(listenerPropertyGroup);

auto validator = boost::make_shared<ArrayBoundedValidator<int>>();
validator->setLower(1);
declareProperty(make_unique<ArrayProperty<int>>("PeriodList", validator),
"An optional list of periods to load. If blank, all "
"available periods will be loaded. Applied to ISIS histogram"
" data only.");
getPointerToProperty("PeriodList")->setGroup(listenerPropertyGroup);

// Initialize the properties common to LiveDataAlgorithm.
initProps();

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

/**
* After Instrument property is set copy any properties that the instrument's
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

* listener may have to this algorithm.
*/
void StartLiveData::afterPropertySet(const std::string &propName) {
if (propName == "Instrument") {
// Properties of old listener, if any, need to be removed
removeListenerProperties();

// Get of instance of listener for this instrument
auto listener = LiveListenerFactory::Instance().create(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

getPropertyValue(propName), false);

// Copy over properties of new listener to this algorithm
copyListenerProperties(listener);
}
}

/**
* Copies properties from an ILiveListener to this algorithm. This makes them
* appear in the "listener properties" group on the StartLiveData custom dialog.
*
* @param listener ILiveListener from which to copy properties
*/
void StartLiveData::copyListenerProperties(
const boost::shared_ptr<ILiveListener> &listener) {
// Add clones of listener's properties to this algorithm
for (auto listenerProp : listener->getProperties()) {
auto prop = std::unique_ptr<Property>(listenerProp->clone());
prop->setGroup(listenerPropertyGroup);
declareProperty(std::move(prop));
}
}

/**
* Removes previously copied ILiveListener properties.
*/
void StartLiveData::removeListenerProperties() {
// Remove all properties tagged with the listener property group
for (auto prop : getProperties()) {
if (prop->getGroup() == listenerPropertyGroup) {
removeProperty(prop->name());
}
}
}

//----------------------------------------------------------------------------------------------
/** Execute the algorithm.
*/
Expand Down
66 changes: 64 additions & 2 deletions Framework/PythonInterface/mantid/simpleapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@

#------------------------ Specialized function calls --------------------------
# List of specialized algorithms
__SPECIALIZED_FUNCTIONS__ = ["Load", "CutMD", "RenameWorkspace"]
__SPECIALIZED_FUNCTIONS__ = ["Load", "StartLiveData", "CutMD", "RenameWorkspace"]
# List of specialized algorithms
__MDCOORD_FUNCTIONS__ = ["PeakIntensityVsRadius", "CentroidPeaksMD","IntegratePeaksMD"]
# The "magic" keyword to enable/disable logging
Expand Down Expand Up @@ -215,6 +215,66 @@ def LoadDialog(*args, **kwargs):
algm.execute()
return algm

#------------------------------------------------------------------------------

def StartLiveData(*args, **kwargs):
"""
StartLiveData dynamically adds the properties of the specific LiveListener
that is used to itself, to allow usage such as the following:

StartLiveData(Instrument='ISIS_Histogram', ...
PeriodList=[1,3], SpectraList=[2,4,6])

Where PeriodList and SpectraList are properties of the ISISHistoDataListener
rather than of StartLiveData. For StartLiveData to know those are valid
properties, however, it first needs to know what the Instrument is.

This is a similar situation as in the Load algorithm, where the Filename
must be provided before other properties become available, and so it is
solved here in the same way.
"""
instrument, = _get_mandatory_args('StartLiveData', ["Instrument"], *args, **kwargs)

# Create and execute
(_startProgress, _endProgress, kwargs) = extract_progress_kwargs(kwargs)
algm = _create_algorithm_object('StartLiveData',
startProgress=_startProgress,
endProgress=_endProgress)
_set_logging_option(algm, kwargs)
try:
algm.setProperty('Instrument', instrument) # Must be set first
except ValueError as ve:
raise ValueError('Problem when setting Instrument. This is the detailed error '
'description: ' + str(ve))

# Remove from keywords so it is not set twice
try:
del kwargs['Instrument']
except KeyError:
pass

# LHS Handling currently unsupported for StartLiveData
lhs = _kernel.funcinspect.lhs_info()
if lhs[0] > 0: # Number of terms on the lhs
raise RuntimeError("Assigning the output of StartLiveData is currently "
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. Unwrapping is not working properly because of the optional "AccumulationWorkspace" output, and there's no easy fix for this.
  2. 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.
  3. 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.
  4. Since OutputWorkspace is being periodically replaced in the ADS, any reference to it you get in Python is invalidated shortly after you get it.

"unsupported due to limitations of the simpleapi. "
"Please call StartLiveData without assigning it to "
"to anything.")

lhs_args = _get_args_from_lhs(lhs, algm)
final_keywords = _merge_keywords_with_lhs(kwargs, lhs_args)

# Check for any properties that aren't known and warn they will not be used
for key in list(final_keywords.keys()):
if key not in algm:
logger.warning("You've passed a property (%s) to StartLiveData() that doesn't apply to this Instrument." % key)
del final_keywords[key]

set_properties(algm, **final_keywords)
algm.execute()

return _gather_returns("StartLiveData", lhs, algm)

#---------------------------- Fit ---------------------------------------------

def fitting_algorithm(f):
Expand Down Expand Up @@ -702,7 +762,7 @@ def _merge_keywords_with_lhs(keywords, lhs_args):
final_keywords.update(keywords)
return final_keywords

def _gather_returns(func_name, lhs, algm_obj, ignore_regex=[]):
def _gather_returns(func_name, lhs, algm_obj, ignore_regex=None):
"""Gather the return values and ensure they are in the
correct order as defined by the output properties and
return them as a tuple. If their is a single return
Expand All @@ -713,6 +773,8 @@ def _gather_returns(func_name, lhs, algm_obj, ignore_regex=[]):
:param algm_obj: An executed algorithm object.
:param ignore_regex: A list of strings containing regex expressions to match against property names that will be ignored & not returned.
"""
if ignore_regex is None: ignore_regex = []

import re
def ignore_property(name, ignore_regex):
for regex in ignore_regex:
Expand Down
23 changes: 23 additions & 0 deletions docs/source/algorithms/StartLiveData-v1.rst
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,29 @@ simply calls :ref:`algm-LoadLiveData` at a fixed interval.
Instructions for setting up a "fake" data stream are found `here
<http://www.mantidproject.org/MBC_Live_Data_Simple_Examples>`__.

Listener Properties
###################

Specific LiveListeners may provide their own properties, in addition to
properties provided by StartLiveData. For convenience and accessibility, these
properties are made available through StartLiveData as well.

In the StartLiveData algorithm dialog, a group box called "Listener Properties"
will appear at the bottom of the sidebar on the left, if the currently selected
listener provides additional properties.

In the Python API, these listener properties may also be set as keyword
arguments when calling StartLiveData. For example, in this code snippet:

.. code-block:: python

StartLiveData(Instrument='ISIS_Histogram', OutputWorkspace='wsOut', UpdateEvery=1,
AccumulationMethod='Replace', PeriodList=[1,3], SpectraList=[2,4,6])

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good documentation!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! :)

Live Plots
##########

Expand Down
2 changes: 2 additions & 0 deletions docs/source/release/v3.9.0/framework.rst
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Improved
########

- :ref:`CalculateFlatBackground <algm-CalculateFlatBackground>` has now a new mode 'Moving Average' which takes the minimum of a moving window average as the flat background.
- :ref:`StartLiveData <algm-StartLiveData>` and its dialog now support dynamic listener properties, based on the specific LiveListener being used.

Deprecated
##########
Expand All @@ -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 value of the "Instrument" parameter.

Bug Fixes
---------
Expand Down