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

Enabled/VisibleWhenProperty - Multiple conditions #19264

Merged

Conversation

DavidFair
Copy link
Contributor

@DavidFair DavidFair commented Mar 29, 2017

PR #19244 needs to be merged first

Description of work.
This PR allows EnableWhenProperty and VisibleWhenProperty to use logical operators and or xor with existing Enable/VisibleWhenProperty objects. I've tried to match the style of the existing interface from Python and C++.

The work completed breaks down as follows:

  • Added new constructor and methods to take two existing objects and combine their results with the specified logic operator
  • Made both Enable/VisibleWhenProperty copyable - this means that have the flexibility to reuse the object (most likely from Python) to either combinations and set the original condition for single properties
  • Added error checking. Previously it returned true if there was an error
  • Fixed algorithms which were unintentionally broken due to the above (wrong property name...etc.)
  • Cleaned up the unit tests associated with these classes
  • Exposed changes to Python API
  • Unit tests on Python and C++ side
  • Cleanup of Doxygen within the header and cpp files

Examples


To use a combination of properties to determine another properties state you would do something as follows in C++:

auto propOne = make_shared<EnabledWhenProperty>("MyPropName", IS_EQUAL_TO, "foo");
auto propTwo = make_shared<EnabledWhenProperty>("PropNameTwo", IS_EQUAL_TO, "bar");
// Can either use std::move or trigger a copy
// Creating a copy:
auto combination = make_shared<EnabledWhenProperty>(propOne, propTwo, AND);
// Using std::move
auto combination = make_shared<EnabledWhenProperty>(std::move(propOne), std::move(propTwo), AND)
// To set the property
myAlg.setPropertySettings("MyDisabledPropName", combination);

For Python:

from mantid.kernel import EnabledWhenProperty, PropertyCriterion, LogicOperator

prop_one = EnabledWhenProperty("propOne", PropertyCriterion.IsDefault)
prop_two = EnabledWhenProperty("propTwo", PropertyCriterion.IsEqualTo, "val")
combination = EnabledWhenProperty(prop_one, prop_two, LogicOperator.Or)  # Could use And / Xor
self.setPropertySettings("MyEnabledProperty", combination)

To test:

  • Open IPython window, copy and run the following code
from mantid.kernel import EnabledWhenProperty, PropertyCriterion, LogicOperator
from mantid.api import PythonAlgorithm

class EnabledWhenPropTest(PythonAlgorithm):
    def category(self):
        return("PR Test")
    
    def PyInit(self):
        self.declareProperty("PropertyOne", 1)
        self.declareProperty("PropertyTwo", 2)
        
        propOneCondition = EnabledWhenProperty("PropertyOne", PropertyCriterion.IsDefault)
        propTwoCondition = EnabledWhenProperty("PropertyTwo", PropertyCriterion.IsEqualTo, "2")
        combination = EnabledWhenProperty(propOneCondition, propTwoCondition, LogicOperator.And)
        
        self.declareProperty("ToggledProp", 987)
        self.setPropertySettings("ToggledProp", combination)
        
        
    def PyExec(self):
        pass
        
        
AlgorithmFactory.subscribe(EnabledWhenPropTest)
  • A new algorithm called EnabledWhenPropTest should appear
  • Changing the value either value should disable the final box
  • Restoring the original values should re-enable the final box
  • Try using different logic operators such as or / xor

For VisibleWhenProperty

from mantid.kernel import VisibleWhenProperty, PropertyCriterion, LogicOperator
from mantid.api import PythonAlgorithm

class VisibleWhenPropTest(PythonAlgorithm):
    def category(self):
        return("PR Test")
    
    def PyInit(self):
        self.declareProperty("PropertyOne", 1)
        self.declareProperty("PropertyTwo", 2)
        
        propOneCondition = VisibleWhenProperty("PropertyOne", PropertyCriterion.IsDefault)
        propTwoCondition = VisibleWhenProperty("PropertyTwo", PropertyCriterion.IsEqualTo, "2")
        combination = VisibleWhenProperty(propOneCondition, propTwoCondition, LogicOperator.And)
        
        self.declareProperty("ToggledProp", 987)
        self.setPropertySettings("ToggledProp", combination)
        
        
    def PyExec(self):
        pass
        
        
AlgorithmFactory.subscribe(VisibleWhenPropTest)

Fixes #19257 .

Does not need to be in the release notes. - This change is only visible internally.
However may be worth making developers aware as I'm sure it will be handy for some algorithms.


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.

@DavidFair DavidFair added Framework Issues and pull requests related to components in the Framework Extra Attention Testers and Gate keepers should pay extra attention as this affects core aspects. labels Mar 29, 2017
@DavidFair DavidFair added this to the Release 3.10 milestone Mar 29, 2017
@louisemccann louisemccann self-assigned this Apr 3, 2017
a = VisibleWhenProperty("PropA", PropertyCriterion.IsDefault)
b = VisibleWhenProperty("PropB", PropertyCriterion.IsDefault)
result = VisibleWhenProperty(a, b, LogicOperator.And)
self.assertIsNotNone(result)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add test for all logic operators as in EnabledWhenProperty test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't test from Python if these actually work as we would need to subscribe an algorithm and manipulate the properties.
Instead the Python tests just check we can construct it and the C++ unit tests test all combinations of operators work.
I'll add this note to the unit tests on the Python side

@louisemccann
Copy link
Contributor

LGTM :shipit: when all tests pass

@AndreiSavici AndreiSavici merged commit e4105b2 into master Apr 21, 2017
@AndreiSavici AndreiSavici deleted the 19257_EnabledWhenProperty_accept_multiple_properties branch April 21, 2017 19:27
@DavidFair DavidFair moved this from PR to Done in ISIS Powder Diffraction Apr 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extra Attention Testers and Gate keepers should pay extra attention as this affects core aspects. Framework Issues and pull requests related to components in the Framework
Development

Successfully merging this pull request may close these issues.

None yet

3 participants