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
1 change: 1 addition & 0 deletions Framework/LiveData/inc/MantidLiveData/StartLiveData.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class DLLExport StartLiveData : public LiveDataAlgorithm {
private:
void init() override;
void exec() override;
void afterPropertySet(const std::string &) override;
};

} // namespace LiveData
Expand Down
47 changes: 32 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,38 @@ 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") {
// Remove old listener's properties
const std::vector<Property *> existingProps = getProperties();
for (auto existingProp : existingProps) {
if (existingProp->getGroup() == listenerPropertyGroup) {
removeProperty(existingProp->name());
}
}

// Add new listener's properties
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);
auto propertyManagerListener =
boost::dynamic_pointer_cast<IPropertyManager>(listener);

if (propertyManagerListener) {
const std::vector<Property *> listenerProps =
propertyManagerListener->getProperties();
for (auto listenerProp : listenerProps) {
auto prop = std::unique_ptr<Property>(listenerProp->clone());
prop->setGroup(listenerPropertyGroup);
declareProperty(std::move(prop));
}
}
}
}

//----------------------------------------------------------------------------------------------
/** 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
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 Instrument parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 adding a section on listener properties to StartLiveData rst.


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