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

jmarrec · 2022-06-29T11:25:36Z

Pull request overview

Fixes Add minimum/maximum values to numeric OSArguments #4551

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

…instead of the deprecated Ruleset one

…in/max value now will show in measure.xml

… minimum finite value (2.2250738585072014e-308 not `- std::numeric_limits<T>::max()`

jmarrec · 2022-06-29T11:25:55Z

src/cli/measure_manager.rb

@@ -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)


Modernize our script to not use the deprecated namespace

jmarrec · 2022-06-29T11:26:33Z

src/cli/measure_manager.rb

+        if argument.hasDomain
+          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


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

jmarrec · 2022-06-29T11:27:37Z

src/measure/OSArgument.cpp

@@ -763,7 +763,7 @@ namespace measure {
      return false;
    }

-    double minValue = std::numeric_limits<double>::min();
+    double minValue = std::numeric_limits<double>::lowest();


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

jmarrec · 2022-06-29T11:28:42Z

src/measure/OSMeasureInfoGetter.cpp

+        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);
+          }
+        }


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

jmarrec · 2022-06-29T11:29:34Z

src/measure/OSRunner.cpp

+          } 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);
+            }


Do the actual validation of the Double/Integer domain inside valdiateUserArguments => registers an actual error if out of bounds

jmarrec · 2022-06-29T11:29:45Z

src/measure/test/OSMeasure_GTest.cpp

+    OSArgument arg = OSArgument::makeDoubleArgument("double_arg", true);
+    arg.setMaxValue(10.0);
+    result.push_back(arg);
+
+    arg = OSArgument::makeIntegerArgument("int_arg", true);
+    arg.setMinValue(0);
+    result.push_back(arg);


jmarrec · 2022-06-29T11:31:01Z

src/measure/test/OSMeasure_GTest.cpp

+  // call with a good value
+  double_arg.setValue(-1.0);
+  int_arg.setValue(1.0);
+  EXPECT_TRUE(script.run(model, runner, user_arguments));
+  WorkflowStepResult result = runner.result();
+  ASSERT_TRUE(result.stepResult());
+  EXPECT_EQ(StepResult::Success, result.stepResult()->value());
+  EXPECT_EQ(0u, result.stepErrors().size());
+  EXPECT_EQ(0u, result.stepWarnings().size());
+
+  // Out of bound value for double_arg
+  runner.reset();
+  double_arg.setValue(100.0);
+  EXPECT_FALSE(script.run(model, runner, user_arguments));
+  result = runner.result();
+  ASSERT_TRUE(result.stepResult());
+  EXPECT_EQ(StepResult::Fail, result.stepResult()->value());
+  ASSERT_EQ(1u, result.stepErrors().size());
+  EXPECT_EQ(
+    fmt::format("Double User argument 'double_arg' has a value '100' that is not in the domain [{}, 10].", std::numeric_limits<double>::lowest()),
+    result.stepErrors()[0]);
+  EXPECT_EQ(0u, result.stepWarnings().size());
+
+  // Out of bound value for int_arg
+  runner.reset();
+  double_arg.setValue(1.0);
+  int_arg.setValue(-3);
+  EXPECT_FALSE(script.run(model, runner, user_arguments));
+  result = runner.result();
+  ASSERT_TRUE(result.stepResult());
+  EXPECT_EQ(StepResult::Fail, result.stepResult()->value());
+  ASSERT_EQ(1u, result.stepErrors().size());
+  EXPECT_EQ("Integer User argument 'int_arg' has a value '-3' that is not in the domain [0, 2147483647].", result.stepErrors()[0]);
+  EXPECT_EQ(0u, result.stepWarnings().size());


As can be seen, it does capture the issue of the out of bounds.

If you run the test with e2d00da (before I changed the std::numeric_limits<T>::min() to std::numeric_limits<T>::lowest()) it fails (I did that fix before extending the tests but reordered the commits to show the issue)

ci-commercialbuildings · 2022-06-29T11:39:02Z

CI Results for 058d347:

Ubuntu 18.04:
- OpenStudio-3.4.1-alpha+058d3476cc-Ubuntu-18.04.deb
- OpenStudio-3.4.1-alpha+058d3476cc-Ubuntu-18.04.tar.gz
Ubuntu 20.04:
Darwin:
- OpenStudio-3.4.1-alpha+058d3476cc-Darwin-x86_64.dmg
- OpenStudio-3.4.1-alpha+058d3476cc-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.4.1-alpha+058d3476cc-Windows.exe
- OpenStudio-3.4.1-alpha+058d3476cc-Windows.tar.gz
Code Coverage:

jmarrec · 2022-06-30T06:18:02Z

@DavidGoldwasser Could you take a look at this one please? And confirm adding the min_value / max_value to the measure.xml is ok with you?

One thing I also wanted to do was to replace JSON.generate by JSON.pretty_generate for --compute-arguments and --update (not update_all), so the resulting JSON is actually readable by a human instead of being a condensed one-liner.

OpenStudio/src/cli/openstudio_cli.rb

Line 1483 in 0e7dda7

safe_puts JSON.generate(hash)

OpenStudio/src/cli/openstudio_cli.rb

Line 1543 in 0e7dda7

safe_puts JSON.generate(hash)

Thoughts anyone?

joseph-robertson · 2022-07-27T18:34:23Z

src/cli/measure_manager.rb

@@ -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


Does it still have domain if either min or max are set but not both?

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()]

joseph-robertson · 2022-07-27T18:35:53Z

src/cli/measure_manager.rb

-
+        if argument.hasDomain
+          min, max = argument.domainAsInteger
+          if min != -2147483648


I'm confused what the purpose of this is? Same for above with 1e308.

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

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?

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

joseph-robertson · 2022-07-27T18:39:38Z

src/measure/test/OSMeasure_GTest.cpp

+    result.push_back(arg);
+
+    arg = OSArgument::makeIntegerArgument("int_arg", true);
+    arg.setMinValue(0);


Test with a negative?

jmarrec added 6 commits June 29, 2022 12:04

Modernize measure_manager by using the OpenStudio::Measure namespace …

69e0c68

…instead of the deprecated Ruleset one

cf #4551 - capture domain for int/double OSArguments

a66b516

(+ use fmt::format instead of stringstream where not needed)

9f5f23f

Additional: fix todo: Pass domain from a measure to BCLMeasure: the m…

a6be9f6

…in/max value now will show in measure.xml

Add a test for Double/Integer OSArgument domain validation

e2d00da

std::numeric_limits<T>::min() gives you the smallest positive value =…

058d347

… minimum finite value (2.2250738585072014e-308 not `- std::numeric_limits<T>::max()`

jmarrec added component - Measures component - Measure Manager Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Jun 29, 2022

jmarrec self-assigned this Jun 29, 2022

jmarrec commented Jun 29, 2022

View reviewed changes

jmarrec requested review from joseph-robertson and tijcolem June 29, 2022 11:31

jmarrec requested a review from DavidGoldwasser June 30, 2022 06:13

jmarrec added this to the OpenStudio SDK 3.5.0 milestone Jul 26, 2022

joseph-robertson reviewed Jul 27, 2022

View reviewed changes

tijcolem removed request for DavidGoldwasser and tijcolem July 29, 2022 16:30

joseph-robertson self-requested a review August 3, 2022 01:20

joseph-robertson approved these changes Aug 3, 2022

View reviewed changes

joseph-robertson merged commit 31226fe into develop Aug 12, 2022

joseph-robertson deleted the 4551_Measure_Arg_domain branch August 12, 2022 20:33

joseph-robertson mentioned this pull request May 1, 2024

Use measure argument setMinValue and setMaxValue NREL/OpenStudio-HPXML#1702

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmarrec commented Jun 29, 2022

jmarrec Jun 29, 2022

jmarrec Jun 29, 2022

jmarrec Jun 29, 2022

jmarrec Jun 29, 2022

jmarrec Jun 29, 2022

jmarrec Jun 29, 2022

jmarrec Jun 29, 2022

ci-commercialbuildings commented Jun 29, 2022 •

edited

jmarrec commented Jun 30, 2022

joseph-robertson Jul 27, 2022

jmarrec Jul 28, 2022

joseph-robertson Jul 27, 2022

jmarrec Jul 28, 2022 •

edited

joseph-robertson Jul 28, 2022

jmarrec Jul 29, 2022 •

edited

joseph-robertson Jul 27, 2022

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

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

Conversation

jmarrec commented Jun 29, 2022

Pull request overview

Pull Request Author

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Jun 29, 2022 • edited

jmarrec commented Jun 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec Jul 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec Jul 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Jun 29, 2022 •

edited

jmarrec Jul 28, 2022 •

edited

jmarrec Jul 29, 2022 •

edited