Add native support for measure argument methods in OsLib_HelperMethods, try 2 #5135

joseph-robertson · 2024-04-02T20:47:43Z

Pull request overview

Addresses Add native support for measure argument methods in OsLib_HelperMethods #4993.

Add methods that return Json::Value:

runner.getArgumentValues(script_arguments, user_arguments)
runner.getPastStepValuesForMeasure(measure_name)
runner.getPastStepValuesForName(step_name)

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

…ents.

ci-commercialbuildings · 2024-04-03T22:36:37Z

CI Results for d30c867:

Ubuntu 20.04:
Ubuntu 22.04:
- OpenStudio-3.8.0-beta+d30c867463-Ubuntu-22.04-x86_64.deb
- OpenStudio-3.8.0-beta+d30c867463-Ubuntu-22.04-x86_64.tar.gz
Darwin:
- OpenStudio-3.8.0-beta+d30c867463-Darwin-x86_64.dmg
- OpenStudio-3.8.0-beta+d30c867463-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.8.0-beta+d30c867463-Windows.exe
- OpenStudio-3.8.0-beta+d30c867463-Windows.tar.gz
Code Coverage:

jmarrec · 2024-04-11T07:05:50Z

src/measure/OSArgument.cpp

+      if (hasValue()) {
+        root["value"] = valueAsBool();
+      }


I'm very puzzled why I didn't do hasValue() orginally... I can't recall if this is a mistake or if I had a reason.

ANyways, we should ladder the ifs

if (hasValue()) { ... } else if (hasDefaultValue()) { ... }

OH. Nevermind. I didn't see we're writing to two different keys, value and default_value

Right ok, I was just mimic'ing this

OpenStudio/ruby/engine/measure_manager.rb

Lines 370 to 427 in d30c867

def get_arguments_from_measure(measure_dir, measure)

result = []

begin

# this type was not wrapped with SWIG until OS 1.11.2

measure.arguments.each do |argument|

type = argument.type

arg = {}

arg[:name] = argument.name

arg[:display_name] = argument.displayName

arg[:description] = argument.description.to_s

arg[:type] = argument.type

arg[:required] = argument.required

arg[:model_dependent] = argument.modelDependent

case type

when 'Boolean'

if argument.defaultValue.is_initialized

default_value = argument.defaultValue.get

arg[:default_value] = (default_value.downcase == "true")

end

when 'Double'

arg[:units] = argument.units.get if argument.units.is_initialized

arg[:default_value] = argument.defaultValue.get.to_f if argument.defaultValue.is_initialized

arg[:min_value] = argument.minValue.get.to_f if argument.minValue.is_initialized

arg[:max_value] = argument.maxValue.get.to_f if argument.maxValue.is_initialized

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

when 'Choice'

arg[:default_value] = argument.defaultValue.get if argument.defaultValue.is_initialized

arg[:choice_values] = []

argument.choiceValues.each {|value| arg[:choice_values] << value}

arg[:choice_display_names] = []

argument.choiceDisplayNames.each {|value| arg[:choice_display_names] << value}

when 'Path'

arg[:default_value] = argument.defaultValue.get if argument.defaultValue.is_initialized

end

result << arg

end

rescue

info = get_measure_info(measure_dir, measure, "", OpenStudio::Model::OptionalModel.new, OpenStudio::OptionalWorkspace.new)

return get_arguments_from_measure_info(info)

end

return force_encoding(result, 'utf-8')

end

Alright, this seems just fine to me.

When using a BCLMeasure to inspect arguments, the JSON is unchanged because the OSArgument does not have a value.

jmarrec · 2024-04-11T07:47:11Z

src/measure/test/OSRunner_GTest.cpp

+  EXPECT_EQ(1.0, d);
+
+  double d2 = runner.getDoubleArgumentValue(requiredDoubleArgument2.name(), argumentMap);
+  EXPECT_EQ(234892384234.39485923845834534, d2);


Suggested change

EXPECT_EQ(234892384234.39485923845834534, d2);

EXPECT_DOUBLE_EQ(234892384234.39485923845834534, d2);

jmarrec · 2024-04-11T07:48:09Z

src/measure/test/OSRunner_GTest.cpp

+  optionalChoiceArgument2.setValue(openstudio::toString(boiler2.handle()));
+  argumentVector.push_back(optionalChoiceArgument2);
+
+  std::map<std::string, OSArgument> argumentMap = convertOSArgumentVectorToMap(argumentVector);


You should emplace an extra arg in there for testing purposes

jmarrec · 2024-04-11T07:50:24Z

src/measure/OSRunner.cpp

@@ -784,6 +786,68 @@ namespace measure {
    return result;
  }

+  Json::Value OSRunner::getArgumentValues(std::vector<OSArgument>& script_arguments, const std::map<std::string, OSArgument>& user_arguments) {
+    Json::Value argument_values;
+    std::string name;


scoping. declare it as const std::string name inside the loop, it's safer

jmarrec · 2024-04-11T07:58:57Z

src/measure/OSRunner.cpp

@@ -17,6 +17,8 @@
 #include "../utilities/core/Assert.hpp"
 #include "../utilities/core/PathHelpers.hpp"

+#include <boost/lexical_cast.hpp>


jmarrec · 2024-04-11T08:07:18Z

src/measure/OSRunner.cpp

@@ -784,6 +786,68 @@ namespace measure {
    return result;
  }

+  Json::Value OSRunner::getArgumentValues(std::vector<OSArgument>& script_arguments, const std::map<std::string, OSArgument>& user_arguments) {


So, I'm following https://github.com/NREL/openstudio-extension-gem/blob/bb25c593c6fd3eab214ec92d7d1d5e7625937e2e/lib/openstudio/extension/core/os_lib_helper_methods.rb#L30-L67

But I think it should be different. What I think it should do is, to also handle optionals:

for each measure_arg in script_arguments: if user_argument includes it, and it has a value: use it else if the **measure_arg** has default value: use that, because the user_arguments could theoretically have had default_value modifications else: empty

jmarrec · 2024-04-11T08:43:48Z

src/measure/OSArgument.cpp

+      if (hasValue()) {
+        root["value"] = openstudio::toString(valueAsPath());
+      }


The more I think about it, the more convinced I become that we should just have some convenience functions in OSArgument

Json::Value valueAsJSON() const; Json::Value defaultValueAsJSON() const; Json::Value domainAsJSON() const;

They should be exposed to the bindings, and would convert nicely to Ruby/Python type. Maybe we even go crazy and call them just value() / defaultValue

Also, I think the crazyness we go through when passing arguments to the run method has to go. We should be able to just pass a ruby hash to it, instead of sprinkling every damn measure test with some nastinesss like

OpenStudio/src/utilities/bcl/templates/ModelMeasure/tests/model_measure_test.rb

Lines 42 to 55 in b809318

argument_map = OpenStudio::Measure.convertOSArgumentVectorToMap(arguments)

# create hash of argument values

args_hash = {}

args_hash['space_name'] = ''

# populate argument with specified hash value if specified

arguments.each do |arg|

temp_arg_var = arg.clone

if args_hash.key?(arg.name)

assert(temp_arg_var.setValue(args_hash[arg.name]))

end

argument_map[arg.name] = temp_arg_var

end

jmarrec · 2024-04-11T09:35:49Z

src/measure/OSRunner.cpp

+    return argument_values;
+  }
+
+  Json::Value OSRunner::getPastStepValuesForMeasure(const std::string& measure_name) {


I'm still confused about the difference between the getPastStepValuesForMeasure and getPastStepValuesForName, but I'll figure that out.

The above is mostyl due to how unreadable both methods are. The flow logic places an unnecessarily high cognitive burden on the reader. I'll revamp

Both of these should be marked const.

Generally speaking, the entire existing OSRunner class is completely messed up as far as const-correctness and staticness of methods that can be (like getOptionalXXXArgument), but I'm wary of changing this since it's an ABI break

jmarrec · 2024-04-11T09:39:43Z

src/measure/OSRunner.cpp

@@ -784,6 +786,68 @@ namespace measure {
    return result;
  }

+  Json::Value OSRunner::getArgumentValues(std::vector<OSArgument>& script_arguments, const std::map<std::string, OSArgument>& user_arguments) {


This should be static actually.

…S >= 10.14 supports it, and E+ is 10.15 minimum anyways

…static as it should

…same

* Try to align getPastStepValuesForName on check_upstream_measure_for_arg in what it picks for measure name * getPastStepValuesForMeasure: allow matching on either the WorkflowStep's measureDirName or name, or the WorkflowStepResult's measureName * Scope the test blocks for readability and avoiding side effects (less chances of making a mistake)

…rn defaultValue if value not set

…true * Move logic to Variant::isTruish to determine if a variant is trueish (whether that's bool true, str = "true", int != 0, double != 0.0 * Add a OSRunner::prepareForMeasureRun() that takes no arg, protected, and friend OSWorkflow so we can use it without the penatly of instantiating an OSMeasure * In Apply Measure, when __SKIP__, correctly call prepareForMEasureRun before incrementing the step. And do load the BCLMeasure to write the resultInfo like the workflow-gem was doing

…ap<string, int> as swig template globally I kinda like the usability ``` OpenStudio::StepResult.getLookupMap => std::map<std::string,int,std::less< std::string >,std::allocator< std::pair< std::string const,int > > > {"FAIL"=>1,"NA"=>-1,"NOTAPPLICABLE"=>-1,"PASS"=>0,"SKIP"=>-2,"SKIPPED"=>-2,"SUCCESS"=>0} OpenStudio::StepResult.getLookupMap.to_h => {"FAIL"=>1, "NA"=>-1, "NOTAPPLICABLE"=>-1, "PASS"=>0, "SKIP"=>-2, "SKIPPED"=>-2, "SUCCESS"=>0} ``` python ``` In [16]: openstudio.StepResult.getLookupMap().asdict() Out[16]: {'FAIL': 1, 'NA': -1, 'NOTAPPLICABLE': -1, 'PASS': 0, 'SKIP': -2, 'SKIPPED': -2, 'SUCCESS': 0} ```

``` object = OpenStudio::Measure::OSArgument.makeChoiceArgument('object', {'handle1' => 'choice1', 'handle2' => 'choice2'}, true, true) object.choiceValues => ["handle1", "handle2"] object.choiceValueDisplayNames => ["choice1", "choice2"] ```

joseph-robertson added 24 commits February 2, 2024 15:12

Stub new method for getting arguments hash from runner.

7353eff

Formatting.

051db53

Typo.

4656d9d

More typos.

86876f2

Forgot class name.

bd420d1

Add path and default switch.

0ae2615

Merge branch 'develop' into lib-helper-methods

8814174

Try a simple runner test.

edc8531

Formatting.

5387337

Start adding convenience methods for retrieving upstream runner argum…

d7cce0c

…ents.

Formatting.

3456cfd

New upstream convenience method building.

b00f5f8

Get upstream measure args method and test working.

886241f

Remove unused variables.

da0a311

Try json::value instead of std::map.

4966313

Try json nullvalue when optional not initialized.

915cc41

Update runner test.

619d9f7

Try more runner test updates.

86fb042

Try coercing the types.

4841fa8

Rename to getPastValues and get tests passing.

2a2734a

Remove unused method.

234db35

Merge branch 'develop' into lib-helper-methods

a944304

Merge branch 'develop' into lib-helper-methods

e35f048

Update osargument tojson and use that instead.

1a74b8b

joseph-robertson added the Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. label Apr 2, 2024

joseph-robertson self-assigned this Apr 2, 2024

joseph-robertson added 4 commits April 2, 2024 15:27

Try getting value from root directly.

ecc868c

Fix new valueAs getters.

3ab5c97

Use toJSON with workflow step value.

eb6feaf

Key is measure name not step name.

c8c9780

joseph-robertson mentioned this pull request Apr 3, 2024

Add native support for measure argument methods in OsLib_HelperMethods #5089

Merged

26 tasks

joseph-robertson added this to the OpenStudio SDK 3.8.0 milestone Apr 3, 2024

joseph-robertson marked this pull request as ready for review April 3, 2024 20:24

joseph-robertson added 4 commits April 3, 2024 14:12

Update for namespace.

6555f1a

Convert handle to string.

1b178fd

Merge branch 'develop' into lib-helper-methods2

02890e7

Set correct value type and also workspace.

d30c867

joseph-robertson requested a review from jmarrec April 3, 2024 22:47

jmarrec reviewed Apr 11, 2024

View reviewed changes

joseph-robertson marked this pull request as draft April 11, 2024 16:15

jmarrec added 14 commits April 12, 2024 12:36

It's time to remove the std::get_if workaround and use std::get. MacO…

0a793c5

…S >= 10.14 supports it, and E+ is 10.15 minimum anyways

Similarly, use std::visit now, and convert printOSArgumentVariant to …

2a5b5e6

…static as it should

Modernize code in file

1848931

Extend testing a bit

39c8fff

Modernize code for OSRunner. Make the **private** cleanValueName static

7934905

Make new methods const / static for getArgumentValues

d4c0d35

Change the flow logic to be easier to follow - behavior is still the …

66b923d

…same

Add some valueAsJSON throughout.

2f954c2

getArgumentValues: call validateUserArguments and throw if not + retu…

554ccf5

…rn defaultValue if value not set

Avoid comparison error between domain and enum in test (CI runner)

804f1a4

joseph-robertson marked this pull request as ready for review April 12, 2024 20:35

joseph-robertson merged commit d2f0bdd into develop Apr 12, 2024
1 of 6 checks passed

joseph-robertson deleted the lib-helper-methods2 branch April 12, 2024 20:36

joseph-robertson mentioned this pull request Apr 30, 2024

Unexpected behavior of runner.getArgumentValues() #5174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add native support for measure argument methods in OsLib_HelperMethods, try 2 #5135

Add native support for measure argument methods in OsLib_HelperMethods, try 2 #5135

joseph-robertson commented Apr 2, 2024 •

edited by jmarrec

Loading

ci-commercialbuildings commented Apr 3, 2024 •

edited

Loading

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

jmarrec Apr 11, 2024

	def get_arguments_from_measure(measure_dir, measure)
	result = []

	begin
	# this type was not wrapped with SWIG until OS 1.11.2
	measure.arguments.each do \|argument\|
	type = argument.type

	arg = {}
	arg[:name] = argument.name
	arg[:display_name] = argument.displayName
	arg[:description] = argument.description.to_s
	arg[:type] = argument.type
	arg[:required] = argument.required
	arg[:model_dependent] = argument.modelDependent

	case type
	when 'Boolean'
	if argument.defaultValue.is_initialized
	default_value = argument.defaultValue.get
	arg[:default_value] = (default_value.downcase == "true")
	end

	when 'Double'
	arg[:units] = argument.units.get if argument.units.is_initialized
	arg[:default_value] = argument.defaultValue.get.to_f if argument.defaultValue.is_initialized
	arg[:min_value] = argument.minValue.get.to_f if argument.minValue.is_initialized
	arg[:max_value] = argument.maxValue.get.to_f if argument.maxValue.is_initialized

	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

	when 'Choice'
	arg[:default_value] = argument.defaultValue.get if argument.defaultValue.is_initialized
	arg[:choice_values] = []
	argument.choiceValues.each {\|value\| arg[:choice_values] << value}
	arg[:choice_display_names] = []
	argument.choiceDisplayNames.each {\|value\| arg[:choice_display_names] << value}

	when 'Path'
	arg[:default_value] = argument.defaultValue.get if argument.defaultValue.is_initialized
	end

	result << arg
	end
	rescue
	info = get_measure_info(measure_dir, measure, "", OpenStudio::Model::OptionalModel.new, OpenStudio::OptionalWorkspace.new)
	return get_arguments_from_measure_info(info)
	end

	return force_encoding(result, 'utf-8')
	end

	EXPECT_EQ(234892384234.39485923845834534, d2);
	EXPECT_DOUBLE_EQ(234892384234.39485923845834534, d2);

	argument_map = OpenStudio::Measure.convertOSArgumentVectorToMap(arguments)

	# create hash of argument values
	args_hash = {}
	args_hash['space_name'] = ''

	# populate argument with specified hash value if specified
	arguments.each do \|arg\|
	temp_arg_var = arg.clone
	if args_hash.key?(arg.name)
	assert(temp_arg_var.setValue(args_hash[arg.name]))
	end
	argument_map[arg.name] = temp_arg_var
	end

Add native support for measure argument methods in OsLib_HelperMethods, try 2 #5135

Add native support for measure argument methods in OsLib_HelperMethods, try 2 #5135

Conversation

joseph-robertson commented Apr 2, 2024 • edited by jmarrec Loading

Pull request overview

Pull Request Author

Review Checklist

ci-commercialbuildings commented Apr 3, 2024 • edited Loading

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

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

joseph-robertson commented Apr 2, 2024 •

edited by jmarrec

Loading

ci-commercialbuildings commented Apr 3, 2024 •

edited

Loading