-
Notifications
You must be signed in to change notification settings - Fork 184
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
#4551 - Add minimum/maximum values to numeric OSArguments and use it in validateUserArgument #4621
Changes from all commits
69e0c68
a66b516
9f5f23f
a6be9f6
e2d00da
058d347
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,7 +86,7 @@ def initialize(logger=nil) | |
@measures = {} # measure_dir => BCLMeasure | ||
@measure_info = {} # measure_dir => {osm_path => RubyUserScriptInfo} | ||
|
||
eval(OpenStudio::Ruleset::infoExtractorRubyFunction) | ||
eval(OpenStudio::Measure::infoExtractorRubyFunction) | ||
end | ||
|
||
def force_encoding(object, encoding = 'utf-8') | ||
|
@@ -342,7 +342,7 @@ def get_measure(measure_dir, force_reload) | |
|
||
rescue => e | ||
# update error in info | ||
info = OpenStudio::Ruleset::RubyUserScriptInfo.new(e.message) | ||
info = OpenStudio::Measure::OSMeasureInfo.new(e.message) | ||
info.update(result) | ||
end | ||
|
||
|
@@ -358,7 +358,7 @@ def get_measure(measure_dir, force_reload) | |
return result | ||
end | ||
|
||
# returns OpenStudio::Ruleset::RubyUserScriptInfo | ||
# returns OpenStudio::Measure::OSMeasureInfo | ||
def get_measure_info(measure_dir, measure, osm_path, model, workspace) | ||
|
||
result = nil | ||
|
@@ -379,9 +379,9 @@ def get_measure_info(measure_dir, measure, osm_path, model, workspace) | |
# might need some timeouts or additional protection | ||
print_message("Loading measure info for '#{measure_dir}', '#{osm_path}'") | ||
begin | ||
result = OpenStudio::Ruleset.getInfo(measure, model, workspace) | ||
result = OpenStudio::Measure.getInfo(measure, model, workspace) | ||
rescue Exception => e | ||
result = OpenStudio::Ruleset::RubyUserScriptInfo.new(e.message) | ||
result = OpenStudio::Measure::OSMeasureInfo.new(e.message) | ||
end | ||
|
||
@measure_info[measure_dir] = {} if @measure_info[measure_dir].nil? | ||
|
@@ -423,6 +423,8 @@ def get_arguments_from_measure(measure_dir, measure) | |
when 'Integer' | ||
arg[:units] = argument.units.get if argument.units.is_initialized | ||
arg[:default_value] = argument.defaultValue.get.to_i if argument.defaultValue.is_initialized | ||
arg[:min_value] = argument.minValue.get.to_i if argument.minValue.is_initialized | ||
arg[:max_value] = argument.maxValue.get.to_i if argument.maxValue.is_initialized | ||
|
||
when 'String' | ||
arg[:default_value] = argument.defaultValue.get if argument.defaultValue.is_initialized | ||
|
@@ -468,24 +470,42 @@ def get_arguments_from_measure_info(measure_info) | |
elsif type == "Double".to_OSArgumentType | ||
arg[:units] = argument.units.get if argument.units.is_initialized | ||
arg[:default_value] = argument.defaultValueAsDouble if argument.hasDefaultValue | ||
|
||
if argument.hasDomain | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it still have domain if either min or max are set but not both? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Domain is set as |
||
min, max = argument.domainAsDouble | ||
# I'm a bit wary of rounding issues... I think 1e308 instead of | ||
# Float::MAX (1.7976931348623157e+308) is fine for our applications... | ||
if min > -1e308 # -Float::MAX | ||
arg[:min_value] = min | ||
end | ||
if max < 1e308 # Float::MAX | ||
arg[:max_value] = max | ||
end | ||
end | ||
Comment on lines
+473
to
+483
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
elsif type == "Quantity".to_OSArgumentType | ||
arg[:units] = argument.units.get if argument.units.is_initialized | ||
arg[:default_value] = argument.defaultValueAsQuantity.value if argument.hasDefaultValue | ||
|
||
elsif type == "Integer".to_OSArgumentType | ||
arg[:units] = argument.units.get if argument.units.is_initialized | ||
arg[:default_value] = argument.defaultValueAsInteger if argument.hasDefaultValue | ||
|
||
if argument.hasDomain | ||
min, max = argument.domainAsInteger | ||
if min != -2147483648 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused what the purpose of this is? Same for above with 1e308. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same answer as above: when only min or max is provided, one of the domain boundaries is going to be
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if you did indeed set a min less than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how you could do that though. It'd be an overflow error, but mostly you just can't do it, bindings included. cf: $ irb
2.7.2 :002 > 1e308
=> 1.0e+308
2.7.2 :003 > 1e309
=> Infinity |
||
arg[:min_value] = min | ||
end | ||
if max != 2147483648 | ||
arg[:max_value] = max | ||
end | ||
end | ||
elsif type == "String".to_OSArgumentType | ||
arg[:default_value] = argument.defaultValueAsString if argument.hasDefaultValue | ||
|
||
elsif type == "Choice".to_OSArgumentType | ||
arg[:default_value] = argument.defaultValueAsString if argument.hasDefaultValue | ||
arg[:choice_values] = [] | ||
argument.choiceValues.each {|value| arg[:choice_values] << value} | ||
arg[:choice_display_names] = [] | ||
argument.choiceValueDisplayNames.each {|value| arg[:choice_display_names] << value} | ||
arg[:choice_values] = [] | ||
argument.choiceValues.each {|value| arg[:choice_values] << value} | ||
arg[:choice_display_names] = [] | ||
argument.choiceValueDisplayNames.each {|value| arg[:choice_display_names] << value} | ||
|
||
elsif type == "Path".to_OSArgumentType | ||
arg[:default_value] = argument.defaultValueAsPath.to_s if argument.hasDefaultValue | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -763,7 +763,7 @@ namespace measure { | |
return false; | ||
} | ||
|
||
double minValue = std::numeric_limits<double>::min(); | ||
double minValue = std::numeric_limits<double>::lowest(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using Use cf https://godbolt.org/z/K1aq87Yjh
|
||
if (hasDomain() && (m_domainType == OSDomainType::Interval)) { | ||
std::vector<double> domain = domainAsDouble(); | ||
if (domain.size() == 2) { | ||
|
@@ -789,7 +789,7 @@ namespace measure { | |
return false; | ||
} | ||
|
||
int minValue = std::numeric_limits<int>::min(); | ||
int minValue = std::numeric_limits<int>::lowest(); | ||
if (hasDomain() && (m_domainType == OSDomainType::Interval)) { | ||
std::vector<int> domain = domainAsInteger(); | ||
if (domain.size() == 2) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -161,7 +161,27 @@ namespace measure { | |
boost::optional<std::string> minValue; | ||
boost::optional<std::string> maxValue; | ||
if (argument.hasDomain()) { | ||
// TODO | ||
if (argument.type() == OSArgumentType::Integer) { | ||
auto domain = argument.domainAsInteger(); | ||
auto& low = domain.front(); | ||
auto& high = domain.back(); | ||
if (low > std::numeric_limits<int>::lowest()) { | ||
minValue = std::to_string(low); | ||
} | ||
if (high < std::numeric_limits<int>::max()) { | ||
maxValue = std::to_string(high); | ||
} | ||
} else if (argument.type() == OSArgumentType::Double) { | ||
auto domain = argument.domainAsDouble(); | ||
auto& low = domain.front(); | ||
auto& high = domain.back(); | ||
if (low > std::numeric_limits<double>::lowest()) { | ||
minValue = std::to_string(low); | ||
} | ||
if (high < std::numeric_limits<double>::max()) { | ||
maxValue = std::to_string(high); | ||
} | ||
} | ||
Comment on lines
+164
to
+184
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
BCLMeasureArgument bclArgument(argument.name(), argument.displayName(), argument.description(), bclMeasureType, argument.units(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,8 +41,9 @@ | |
#include "../utilities/core/Assert.hpp" | ||
#include "../utilities/core/PathHelpers.hpp" | ||
|
||
#include <stdio.h> | ||
#include <stdlib.h> | ||
#include <fmt/format.h> | ||
#include <cstdio> | ||
#include <cstdlib> | ||
|
||
namespace openstudio { | ||
namespace measure { | ||
|
@@ -393,19 +394,15 @@ namespace measure { | |
void OSRunner::destroyProgressBar() const {} | ||
|
||
bool OSRunner::validateUserArguments(const std::vector<OSArgument>& script_arguments, const std::map<std::string, OSArgument>& user_arguments) { | ||
bool result(true); | ||
std::stringstream ss; | ||
bool result = true; | ||
std::vector<WorkflowStepValue> stepValues; | ||
for (const OSArgument& script_argument : script_arguments) { | ||
auto it = user_arguments.find(script_argument.name()); | ||
if (it == user_arguments.end()) { | ||
// script_argument is not in user_arguments | ||
// this is only okay for purely optional arguments | ||
if (script_argument.required() || script_argument.hasDefaultValue()) { | ||
ss << "Argument " << script_argument.name() << " is required or has a default value, "; | ||
ss << "but is not in user_arguments."; | ||
registerError(ss.str()); | ||
ss.str(""); | ||
registerError(fmt::format("Argument '{}' is required or has a default value, but is not in user_arguments.", script_argument.name())); | ||
result = false; | ||
} | ||
} else { | ||
|
@@ -414,46 +411,37 @@ namespace measure { | |
|
||
// check that names still match | ||
if (user_argument.name() != script_argument.name()) { | ||
ss << "User argument name '" << user_argument.name() << "' does not match map key "; | ||
ss << script_argument.name() << "."; | ||
registerWarning(ss.str()); | ||
ss.str(""); | ||
registerWarning(fmt::format("User argument name '{}' does not match map key ", user_argument.name())); | ||
} | ||
|
||
// check that types still match | ||
if (user_argument.type() != script_argument.type()) { | ||
ss << "User argument type " << user_argument.type().valueName() << " does not match "; | ||
ss << "script argument type " << script_argument.type().valueName() << "."; | ||
registerError(ss.str()); | ||
ss.str(""); | ||
registerError(fmt::format("User argument type {} does not match script argument type {}.", user_argument.type().valueName(), | ||
script_argument.type().valueName())); | ||
result = false; | ||
} | ||
|
||
// check that we have values | ||
if ((script_argument.required()) && !(user_argument.hasValue() || user_argument.hasDefaultValue())) { | ||
ss << "Script argument '" << script_argument.name() << "' is required, "; | ||
ss << "but the user argument does not have a value or default value set."; | ||
registerError(ss.str()); | ||
ss.str(""); | ||
registerError(fmt::format("Script argument '{}' is required, but the user argument does not have a value or default value set.", | ||
script_argument.name())); | ||
result = false; | ||
} | ||
|
||
// check for default value mismatch | ||
if (script_argument.hasDefaultValue() && !user_argument.hasDefaultValue()) { | ||
ss << "Script argument '" << script_argument.name() << "' has a default value, but the "; | ||
ss << "user-supplied version does not."; | ||
registerWarning(ss.str()); | ||
ss.str(""); | ||
registerWarning(fmt::format("Script argument '{}' has a default value, but the user-supplied version does not.", script_argument.name())); | ||
} | ||
if (!script_argument.hasDefaultValue() && user_argument.hasDefaultValue()) { | ||
ss << "Script argument '" << script_argument.name() << "' does not have a default value, "; | ||
ss << "but the user-supplied version does."; | ||
registerWarning(ss.str()); | ||
ss.str(""); | ||
registerWarning( | ||
fmt::format("Script argument '{}' does not have a default value, but the user-supplied version does.", script_argument.name())); | ||
} | ||
if (script_argument.hasDefaultValue() && user_argument.hasDefaultValue() && (user_argument.type() == script_argument.type())) { | ||
ss << "The default value of script argument " << '\n' << script_argument << '\n'; | ||
ss << "does not match that of the corresponding user argument " << '\n' << user_argument << "."; | ||
std::stringstream ss; | ||
ss << "The default value of script argument " << '\n' | ||
<< script_argument << '\n' | ||
<< "does not match that of the corresponding user argument " << '\n' | ||
<< user_argument << "."; | ||
switch (script_argument.type().value()) { | ||
case OSArgumentType::Boolean: | ||
if (user_argument.defaultValueAsBool() != script_argument.defaultValueAsBool()) { | ||
|
@@ -480,23 +468,19 @@ namespace measure { | |
default: | ||
OS_ASSERT(false); | ||
} | ||
ss.str(""); | ||
} | ||
|
||
// check for domain mismatch | ||
if (script_argument.hasDomain() && !user_argument.hasDomain()) { | ||
ss << "Script argument '" << script_argument.name() << "' has a specified domain, but the "; | ||
ss << "user-supplied version does not."; | ||
registerWarning(ss.str()); | ||
ss.str(""); | ||
registerWarning( | ||
fmt::format("Script argument '{}' has a specified domain, but the user-supplied version does not.", script_argument.name())); | ||
} | ||
if (!script_argument.hasDomain() && user_argument.hasDomain()) { | ||
ss << "Script argument '" << script_argument.name() << "' does not have a specified domain, "; | ||
ss << "but the user-supplied version does."; | ||
registerWarning(ss.str()); | ||
ss.str(""); | ||
registerWarning( | ||
fmt::format("Script argument '{}' does not have a specified domain, but the user-supplied version does.", script_argument.name())); | ||
} | ||
if (script_argument.hasDomain() && user_argument.hasDomain() && (user_argument.type() == script_argument.type())) { | ||
std::stringstream ss; | ||
ss << "The domain of script argument " << '\n' << script_argument << '\n'; | ||
ss << "does not match that of the corresponding user argument " << '\n' << user_argument << "."; | ||
switch (script_argument.type().value()) { | ||
|
@@ -532,48 +516,61 @@ namespace measure { | |
default: | ||
OS_ASSERT(false); | ||
} | ||
ss.str(""); | ||
} | ||
|
||
// success, set the values | ||
if (result) { | ||
switch (user_argument.type().value()) { | ||
case OSArgumentType::Boolean: | ||
if (user_argument.hasValue()) { | ||
stepValues.push_back(WorkflowStepValue(user_argument.name(), user_argument.valueAsBool())); | ||
} else if (user_argument.hasDefaultValue()) { | ||
stepValues.push_back(WorkflowStepValue(user_argument.name(), user_argument.defaultValueAsBool())); | ||
} | ||
break; | ||
case OSArgumentType::Double: | ||
if (user_argument.hasValue()) { | ||
stepValues.push_back(WorkflowStepValue(user_argument.name(), user_argument.valueAsDouble())); | ||
} else if (user_argument.hasDefaultValue()) { | ||
stepValues.push_back(WorkflowStepValue(user_argument.name(), user_argument.defaultValueAsDouble())); | ||
} | ||
break; | ||
case OSArgumentType::Integer: | ||
if (user_argument.hasValue()) { | ||
stepValues.push_back(WorkflowStepValue(user_argument.name(), user_argument.valueAsInteger())); | ||
} else if (user_argument.hasDefaultValue()) { | ||
stepValues.push_back(WorkflowStepValue(user_argument.name(), user_argument.defaultValueAsInteger())); | ||
auto typeValue = user_argument.type().value(); | ||
if (typeValue == OSArgumentType::Boolean) { | ||
if (user_argument.hasValue()) { | ||
stepValues.emplace_back(user_argument.name(), user_argument.valueAsBool()); | ||
} else if (user_argument.hasDefaultValue()) { | ||
stepValues.emplace_back(user_argument.name(), user_argument.defaultValueAsBool()); | ||
} | ||
} else if (typeValue == OSArgumentType::Double) { | ||
auto value = user_argument.hasValue() ? user_argument.valueAsDouble() : user_argument.defaultValueAsDouble(); | ||
if (script_argument.hasDomain()) { | ||
// Validate it's in the domain | ||
auto domain = script_argument.domainAsDouble(); | ||
auto& low = domain.front(); | ||
auto& high = domain.back(); | ||
if (value < low || value > high) { | ||
registerError(fmt::format("{} User argument '{}' has a value '{}' that is not in the domain [{}, {}].", | ||
user_argument.type().valueName(), script_argument.name(), value, low, high)); | ||
result = false; | ||
} | ||
break; | ||
case OSArgumentType::String: | ||
case OSArgumentType::Choice: | ||
case OSArgumentType::Path: | ||
if (user_argument.hasValue()) { | ||
stepValues.push_back(WorkflowStepValue(user_argument.name(), user_argument.valueAsString())); | ||
} else if (user_argument.hasDefaultValue()) { | ||
stepValues.push_back(WorkflowStepValue(user_argument.name(), user_argument.defaultValueAsString())); | ||
} | ||
if (result) { | ||
stepValues.emplace_back(user_argument.name(), value); | ||
} | ||
Comment on lines
+530
to
+545
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do the actual validation of the Double/Integer domain inside valdiateUserArguments => registers an actual error if out of bounds |
||
} else if (typeValue == OSArgumentType::Integer) { | ||
auto value = user_argument.hasValue() ? user_argument.valueAsInteger() : user_argument.defaultValueAsInteger(); | ||
if (script_argument.hasDomain()) { | ||
// Validate it's in the domain | ||
auto domain = script_argument.domainAsInteger(); | ||
auto& low = domain.front(); | ||
auto& high = domain.back(); | ||
if (value < low || value > high) { | ||
registerError(fmt::format("{} User argument '{}' has a value '{}' that is not in the domain [{}, {}].", | ||
user_argument.type().valueName(), script_argument.name(), value, low, high)); | ||
result = false; | ||
} | ||
break; | ||
default: | ||
OS_ASSERT(false); | ||
} | ||
if (result) { | ||
stepValues.emplace_back(user_argument.name(), value); | ||
} | ||
} else if ((typeValue == OSArgumentType::String) || (typeValue == OSArgumentType::Choice) || (typeValue == OSArgumentType::Path)) { | ||
if (user_argument.hasValue()) { | ||
stepValues.emplace_back(user_argument.name(), user_argument.valueAsString()); | ||
} else if (user_argument.hasDefaultValue()) { | ||
stepValues.emplace_back(user_argument.name(), user_argument.defaultValueAsString()); | ||
} | ||
} else { | ||
OS_ASSERT(false); | ||
} | ||
} | ||
} // end if result | ||
} | ||
} | ||
} // end for loop on arguments | ||
|
||
if (result) { | ||
for (const auto& stepValue : stepValues) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modernize our script to not use the deprecated namespace