Commit e193fe83 authored by Danny Hindson's avatar Danny Hindson
Browse files

Resolve some unhelpful behaviour of parameters in SetSample

When SetSample was run from the Algorithm dialog in the workbench UI
it wasn't possible to leave some of the dictionary parameters blank.
This is a valid use case for some combinations of parameters

Also when clearing a parameter that previously had a value the
"blanking out" didn't take effect straight away. You had to reopen
the algorithm dialog and try to run the algorithm a second time to
see validation errors relating to empty fields

Fix problem with instrument name in unit test where test failed
if you happened to have your preferred facility set to TEST_LIVE
parent 77a6d888
......@@ -79,6 +79,7 @@ private:
const Kernel::PropertyManager &materialArgs);
Kernel::PropertyManager materialSettingsEnsureLegacyCompatibility(
const Kernel::PropertyManager &materialArgs);
bool isDictionaryPopulated(const Kernel::PropertyManager_const_sptr dict);
};
} // namespace DataHandling
......
......@@ -414,6 +414,18 @@ void SetSample::assertNonNegative(
}
}
/**
* @brief Checks if a json dictionary parameter is populated or not
* @param dict map
*/
bool SetSample::isDictionaryPopulated(const PropertyManager_const_sptr dict) {
bool isPopulated = false;
if (dict)
if (dict->propertyCount() > 0)
isPopulated = true;
return isPopulated;
}
/// Validate the inputs against each other @see Algorithm::validateInputs
std::map<std::string, std::string> SetSample::validateInputs() {
std::map<std::string, std::string> errors;
......@@ -446,7 +458,16 @@ std::map<std::string, std::string> SetSample::validateInputs() {
{&ShapeArgs::HEIGHT, &ShapeArgs::WIDTH, &ShapeArgs::THICK,
&ShapeArgs::RADIUS, &ShapeArgs::INNER_RADIUS, &ShapeArgs::OUTER_RADIUS}};
if (environArgs) {
if (!isDictionaryPopulated(geomArgs) &&
!isDictionaryPopulated(materialArgs) &&
!isDictionaryPopulated(environArgs) &&
!isDictionaryPopulated(canGeomArgs) &&
!isDictionaryPopulated(canMaterialArgs)) {
errors["Geometry"] =
"At least one of the input parameters must be populated";
}
if (isDictionaryPopulated(environArgs)) {
if (!existsAndNotEmptyString(*environArgs, SEArgs::NAME)) {
errors[PropertyNames::ENVIRONMENT] =
"Environment flags require a non-empty 'Name' entry.";
......@@ -455,7 +476,7 @@ std::map<std::string, std::string> SetSample::validateInputs() {
// validate the sample settings, since only the overriding properties
// are specified. Hence we just make sure that whatever is specified is
// at least positive
if (geomArgs) {
if (isDictionaryPopulated(geomArgs)) {
assertNonNegative(errors, *geomArgs, PropertyNames::GEOMETRY,
positiveValues);
}
......@@ -463,22 +484,22 @@ std::map<std::string, std::string> SetSample::validateInputs() {
} else {
// We cannot strictly require geometry and material to be defined
// simultaneously; it can be that one is defined at a later time
if (geomArgs) {
if (isDictionaryPopulated(geomArgs)) {
assertNonNegative(errors, *geomArgs, PropertyNames::GEOMETRY,
positiveValues);
validateGeometry(errors, *geomArgs, PropertyNames::GEOMETRY);
}
if (materialArgs) {
if (isDictionaryPopulated(materialArgs)) {
validateMaterial(errors, *materialArgs, PropertyNames::MATERIAL);
}
}
if (canGeomArgs) {
if (isDictionaryPopulated(canGeomArgs)) {
assertNonNegative(errors, *canGeomArgs, PropertyNames::CONTAINER_GEOMETRY,
positiveValues);
validateGeometry(errors, *canGeomArgs, PropertyNames::CONTAINER_GEOMETRY);
}
if (canMaterialArgs) {
if (isDictionaryPopulated(canMaterialArgs)) {
validateMaterial(errors, *canMaterialArgs,
PropertyNames::CONTAINER_MATERIAL);
}
......@@ -539,15 +560,15 @@ void SetSample::exec() {
// defines a sample geometry then we can process the Geometry flags
// combined with this
const SampleEnvironment *sampleEnviron(nullptr);
if (environArgs) {
if (isDictionaryPopulated(environArgs)) {
sampleEnviron = setSampleEnvironmentFromFile(*experimentInfo, environArgs);
} else if (canGeometryArgs) {
} else if (isDictionaryPopulated(canGeometryArgs)) {
setSampleEnvironmentFromXML(*experimentInfo, canGeometryArgs,
canMaterialArgs);
}
double sampleVolume = 0.;
if (geometryArgs || sampleEnviron) {
if (isDictionaryPopulated(geometryArgs) || sampleEnviron) {
setSampleShape(*experimentInfo, geometryArgs, sampleEnviron);
if (experimentInfo->sample().getShape().hasValidShape()) {
// get the volume back out to use in setting the material
......@@ -557,7 +578,7 @@ void SetSample::exec() {
}
// Finally the material arguments
if (materialArgs) {
if (isDictionaryPopulated(materialArgs)) {
PropertyManager materialArgsCompatible =
materialSettingsEnsureLegacyCompatibility(*materialArgs);
// add the sample volume if it was defined/determined
......
......@@ -760,6 +760,22 @@ public:
void test_flat_plate_holder() {}
void test_Explicit_Blanks_Accepted_For_Dictionary_Parameters() {
// when run from algorithm dialog in UI with some dictionary parameters
// blank
auto inputWS = WorkspaceCreationHelper::create2DWorkspaceBinned(1, 1);
auto sampleShape = ComponentCreationHelper::createSphere(0.5);
sampleShape->setID("mysample");
inputWS->mutableSample().setShape(sampleShape);
auto alg = createAlgorithm(inputWS);
alg->setProperty("Geometry", "");
alg->setProperty("Material", createMaterialProps());
alg->setProperty("Environment", "");
TS_ASSERT_THROWS_NOTHING(alg->execute());
TS_ASSERT(alg->isExecuted());
}
//----------------------------------------------------------------------------
// Failure tests
//----------------------------------------------------------------------------
......@@ -790,6 +806,9 @@ public:
using Mantid::Kernel::PropertyManager;
using StringProperty = Mantid::Kernel::PropertyWithValue<std::string>;
auto inputWS = WorkspaceCreationHelper::create2DWorkspaceBinned(1, 1);
auto testInst = ComponentCreationHelper::createTestInstrumentCylindrical(1);
testInst->setName(m_instName);
inputWS->setInstrument(testInst);
auto alg = createAlgorithm(inputWS);
......@@ -797,7 +816,7 @@ public:
args->declareProperty(std::make_unique<StringProperty>("Name", m_envName),
"");
alg->setProperty("Environment", args);
TS_ASSERT_THROWS(alg->execute(), const std::runtime_error &);
TS_ASSERT_THROWS(alg->execute(), const std::invalid_argument &);
}
void test_Environment_Args_With_Empty_Strings_Invalid() {
......@@ -927,6 +946,19 @@ public:
config.setString("instrumentDefinition.directory", defaultDirs);
}
void test_All_Dictionaries_Empty_Gives_Error() {
using Mantid::Geometry::SampleEnvironment;
using Mantid::Kernel::ConfigService;
auto inputWS = WorkspaceCreationHelper::create2DWorkspaceBinned(1, 1);
auto testInst = ComponentCreationHelper::createTestInstrumentCylindrical(1);
testInst->setName(m_instName);
inputWS->setInstrument(testInst);
auto alg = createAlgorithm(inputWS);
TS_ASSERT_THROWS(alg->execute(), const std::runtime_error &);
}
//----------------------------------------------------------------------------
// Non-test methods
//----------------------------------------------------------------------------
......
......@@ -198,6 +198,8 @@ public:
virtual void removeProperty(const std::string &name,
const bool delproperty = true) = 0;
virtual void resetProperties() = 0;
/** Sets all the declared properties from a string.
@param propertiesString :: A list of name = value pairs separated by a
semicolon
......
......@@ -70,7 +70,7 @@ public:
using IPropertyManager::declareProperty;
void declareOrReplaceProperty(std::unique_ptr<Property> p,
const std::string &doc = "") override;
void resetProperties() override;
// Sets all the declare properties
void setProperties(const std::string &propertiesJson,
const std::unordered_set<std::string> &ignoreProperties =
......
......@@ -43,7 +43,7 @@ public:
// Function to declare properties (i.e. store them)
void declareOrReplaceProperty(std::unique_ptr<Property> p,
const std::string &doc = "") override;
void resetProperties() override;
using IPropertyManager::declareProperty;
// Sets all the declared properties from
......
......@@ -295,6 +295,15 @@ void PropertyManager::declareOrReplaceProperty(std::unique_ptr<Property> p,
m_properties[key] = std::move(p);
}
/** Reset property values back to initial values (blank or default values)
*/
void PropertyManager::resetProperties() {
for (auto &prop : getProperties()) {
if (!prop->isDefault())
prop->setValue(prop->getDefault());
}
}
//-----------------------------------------------------------------------------------------------
/** Set the ordered list of properties by one string of values, separated by
*semicolons.
......
......@@ -57,6 +57,12 @@ void PropertyManagerOwner::declareOrReplaceProperty(std::unique_ptr<Property> p,
m_properties->declareOrReplaceProperty(std::move(p), doc);
}
/** Reset property values back to initial values (blank or default values)
*/
void PropertyManagerOwner::resetProperties() {
m_properties->resetProperties();
}
/** Set the ordered list of properties by one string of values, separated by
*semicolons.
*
......
......@@ -96,11 +96,13 @@ std::string PropertyManagerProperty::setValue(const std::string &strValue) {
value = std::make_shared<PropertyManager>();
(*this) = value;
}
std::string strValueToSet = strValue.empty() ? "{}" : strValue;
std::ostringstream msg;
try {
const std::unordered_set<std::string> ignored;
bool createMissing{true};
value->setProperties(strValue, ignored, createMissing);
value->resetProperties();
value->setProperties(strValueToSet, ignored, createMissing);
m_dataServiceKey.clear();
} catch (std::invalid_argument &exc) {
msg << "Error setting value from string.\n"
......
......@@ -54,4 +54,6 @@ Improvements
Bugfixes
########
- Fix problem with dictionary parameters on :ref:`SetSample <algm-SetSample>` algorithm when running from the algorithm dialog
:ref:`Release 6.1.0 <v6.1.0>`
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment