Repository: NREL/OpenStudio (language: python)

Issue:
Add minimum/maximum values to numeric OSArguments
Enhancement Request
--------------
Extend OSArgument (integer and double types) to allow setting (and getting) minimum/maximum values, and have these displayed in the XML/JSON when arguments are computed.
e.g. I would like to do something like this:
```
    demand_target = OpenStudio::Measure::OSArgument.makeDoubleArgument('demand_target', true)
    demand_target.setDisplayName("Target Demand Limit")
    demand_target.setDescription("Demand response controls will adjust building loads to attempt to maintain this demand level (kW).")
    demand_target.setDefaultValue(100)
    demand_target.setMinValue(0)
    args << demand_target
```
and when running `openstudio measure --compute_arguments` get:
```
"arguments": [
	{
		"name": "demand_target",
		"display_name": "Target Demand Limit",
		"description": "Demand response controls will adjust building loads to attempt to maintain this demand level (kW).",
		"type": "Double",
		"required": true,
		"model_dependent": false,
		"default_value": 100.0,
		"min_value": 0.0
	}
]
```
Note: right now `setMinValue` and `setMaxValue` both exist, but don't seem to do anything useful. This would allow external interfaces to use the JSON to validate measure input before running the measure, and for measure validation within the run method. Could also extend [`runner.validateUserArguments`](https://github.com/NREL/OpenStudio/blob/49f77e7cb61bde928b25f74f8acdbc07a4110d5e/src/measure/OSRunner.cpp#L395) to do the same.


Add minimum/maximum values to numeric OSArguments
Enhancement Request
--------------
Extend OSArgument (integer and double types) to allow setting (and getting) minimum/maximum values, and have these displayed in the XML/JSON when arguments are computed.
e.g. I would like to do something like this:
```
    demand_target = OpenStudio::Measure::OSArgument.makeDoubleArgument('demand_target', true)
    demand_target.setDisplayName("Target Demand Limit")
    demand_target.setDescription("Demand response controls will adjust building loads to attempt to maintain this demand level (kW).")
    demand_target.setDefaultValue(100)
    demand_target.setMinValue(0)
    args << demand_target
```
and when running `openstudio measure --compute_arguments` get:
```
"arguments": [
	{
		"name": "demand_target",
		"display_name": "Target Demand Limit",
		"description": "Demand response controls will adjust building loads to attempt to maintain this demand level (kW).",
		"type": "Double",
		"required": true,
		"model_dependent": false,
		"default_value": 100.0,
		"min_value": 0.0
	}
]
```
Note: right now `setMinValue` and `setMaxValue` both exist, but don't seem to do anything useful. This would allow external interfaces to use the JSON to validate measure input before running the measure, and for measure validation within the run method. Could also extend [`runner.validateUserArguments`](https://github.com/NREL/OpenStudio/blob/49f77e7cb61bde928b25f74f8acdbc07a4110d5e/src/measure/OSRunner.cpp#L395) to do the same.




Discussion / hints:

Fix patch:
diff --git a/src/cli/measure_manager.rb b/src/cli/measure_manager.rb
index afdc5cf2372..78484d742b4 100644
--- a/src/cli/measure_manager.rb
+++ b/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)
   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,7 +470,17 @@ 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
+          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
       elsif type == "Quantity".to_OSArgumentType
         arg[:units] = argument.units.get if argument.units.is_initialized
         arg[:default_value] = argument.defaultValueAsQuantity.value if argument.hasDefaultValue
@@ -476,16 +488,24 @@ def get_arguments_from_measure_info(measure_info)
       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
+            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
diff --git a/src/measure/CMakeLists.txt b/src/measure/CMakeLists.txt
index aed65c9dd28..9a3150b5547 100644
--- a/src/measure/CMakeLists.txt
+++ b/src/measure/CMakeLists.txt
@@ -58,6 +58,7 @@ add_dependencies(${target_name} GenerateIddFactoryRun)
 set(${target_name}_test_depends
   ${COREFOUNDATION_LIBRARY}
   openstudiolib
+  CONAN_PKG::fmt
 )
 
 if( MSVC )
diff --git a/src/measure/OSArgument.cpp b/src/measure/OSArgument.cpp
index 0f8e1402eae..1fcd5482b27 100644
--- a/src/measure/OSArgument.cpp
+++ b/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();
     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) {
diff --git a/src/measure/OSMeasureInfoGetter.cpp b/src/measure/OSMeasureInfoGetter.cpp
index 51ac465a454..6f124635521 100644
--- a/src/measure/OSMeasureInfoGetter.cpp
+++ b/src/measure/OSMeasureInfoGetter.cpp
@@ -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);
+          }
+        }
       }
 
       BCLMeasureArgument bclArgument(argument.name(), argument.displayName(), argument.description(), bclMeasureType, argument.units(),
diff --git a/src/measure/OSRunner.cpp b/src/measure/OSRunner.cpp
index 5119805617c..c35c1fce6fd 100644
--- a/src/measure/OSRunner.cpp
+++ b/src/measure/OSRunner.cpp
@@ -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,8 +394,7 @@ 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());
@@ -402,10 +402,7 @@ namespace measure {
         // 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);
+            }
+          } 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) {


Test patch:
diff --git a/src/measure/test/OSMeasure_GTest.cpp b/src/measure/test/OSMeasure_GTest.cpp
index 9c8a322a297..f2afba9cba5 100644
--- a/src/measure/test/OSMeasure_GTest.cpp
+++ b/src/measure/test/OSMeasure_GTest.cpp
@@ -50,8 +50,11 @@
 
 #include "../../utilities/units/QuantityConverter.hpp"
 
-#include <vector>
+#include <fmt/format.h>
+
+#include <limits>
 #include <map>
+#include <vector>
 
 using namespace openstudio;
 using namespace openstudio::model;
@@ -389,3 +392,92 @@ TEST_F(MeasureFixture, RegisterValueNames) {
   EXPECT_EQ("value_six", attributes[6].name());
   EXPECT_EQ("value_seven", attributes[7].name());
 }
+
+class TestModelUserScriptDomain : public ModelMeasure
+{
+ public:
+  virtual std::string name() const override {
+    return "TestModelUserScriptDomain";
+  }
+
+  virtual std::vector<OSArgument> arguments(const Model& model) const override {
+    std::vector<OSArgument> result;
+
+    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);
+
+    return result;
+  }
+
+  // remove all spaces and add a new one
+  virtual bool run(Model& model, OSRunner& runner, const std::map<std::string, OSArgument>& user_arguments) const override {
+    ModelMeasure::run(model, runner, user_arguments);  // initializes runner
+
+    if (!runner.validateUserArguments(arguments(model), user_arguments)) {
+      return false;
+    }
+
+    return true;
+  }
+};
+
+TEST_F(MeasureFixture, UserScript_TestModelUserScriptDomain) {
+  TestModelUserScriptDomain script;
+  EXPECT_EQ("TestModelUserScriptDomain", script.name());
+
+  Model model;
+
+  std::vector<WorkflowStep> steps;
+  steps.push_back(MeasureStep("dummy"));
+
+  WorkflowJSON workflow;
+  workflow.setWorkflowSteps(steps);
+
+  TestOSRunner runner(workflow);
+  OSArgumentVector definitions = script.arguments(model);
+  std::map<std::string, OSArgument> user_arguments = runner.getUserInput(definitions);
+  ASSERT_EQ(2, user_arguments.size());
+
+  OSArgument& double_arg = user_arguments["double_arg"];
+  OSArgument& int_arg = user_arguments["int_arg"];
+
+  // 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());
+}