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.

Sign up for GitHub

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

Jump to bottom

#4551 - Add minimum/maximum values to numeric OSArguments and use it in validateUserArgument #4621

Merged
merged 6 commits into from
Aug 12, 2022
Merged

#4551 - Add minimum/maximum values to numeric OSArguments and use it in validateUserArgument #4621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
69e0c68
Modernize measure_manager by using the OpenStudio::Measure namespace …
jmarrec Jun 29, 2022
a66b516
cf #4551 - capture domain for int/double OSArguments
jmarrec Jun 29, 2022
9f5f23f
(+ use fmt::format instead of stringstream where not needed)
jmarrec Jun 29, 2022
a6be9f6
Additional: fix todo: Pass domain from a measure to BCLMeasure: the m…
jmarrec Jun 29, 2022
e2d00da
Add a test for Double/Integer OSArgument domain validation
jmarrec Jun 29, 2022
058d347
std::numeric_limits<T>::min() gives you the smallest positive value =…
jmarrec Jun 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 31 additions & 11 deletions src/cli/measure_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator Author

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

end

def force_encoding(object, encoding = 'utf-8')
Expand Down Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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?
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Domain is set as [std::numeric_limits<double>::lowest(), max] if max only is provided, and if min only then [min, std::numeric_limits<double>::max()]

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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

openstudio measure --compute-arguments will now output the min/max.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

@jmarrec jmarrec Jul 28, 2022 •
edited

Choose a reason for hiding this comment

The 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 std::numeric_limits<T>::lowest() or std::numeric_limits<T>::max().

  • In the case of an integer that's +- 2147483648
  • for a double that's +-1.7976931348623157e+308

Copy link
Collaborator

Choose a reason for hiding this comment

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

So if you did indeed set a min less than lowest() or a max greater than max(), wouldn't it not set/write those values?

Copy link
Collaborator Author

@jmarrec jmarrec Jul 29, 2022 •
edited

Choose a reason for hiding this comment

The 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
Expand Down
1 change: 1 addition & 0 deletions src/measure/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ add_dependencies(${target_name} GenerateIddFactoryRun)
set(${target_name}_test_depends
${COREFOUNDATION_LIBRARY}
openstudiolib
CONAN_PKG::fmt
)

if( MSVC )
Expand Down
4 changes: 2 additions & 2 deletions src/measure/OSArgument.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,7 @@ namespace measure {
return false;
}

double minValue = std::numeric_limits<double>::min();
double minValue = std::numeric_limits<double>::lowest();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using min() here means using the smallest positive value. NOT what was expected.

Use lowest() which equals -max()

cf https://godbolt.org/z/K1aq87Yjh

std::numeric_limits<double>::max()   =  1.7976931348623157e+308
std::numeric_limits<double>::min()   =  2.2250738585072014e-308  # POSITIVE minimum finite value
std::numeric_limits<double>::lowest()= -1.7976931348623157e+308

if (hasDomain() && (m_domainType == OSDomainType::Interval)) {
std::vector<double> domain = domainAsDouble();
if (domain.size() == 2) {
Expand All @@ -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) {
Expand Down
22 changes: 21 additions & 1 deletion src/measure/OSMeasureInfoGetter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed this TODO so I also addressed it. It will get passes to BCLMeasureArgument => the measure.xml now will also include the min/max values

image

}

BCLMeasureArgument bclArgument(argument.name(), argument.displayName(), argument.description(), bclMeasureType, argument.units(),
Expand Down
143 changes: 70 additions & 73 deletions src/measure/OSRunner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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()) {
Expand All @@ -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()) {
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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) {
Expand Down