Improve handling of invalid values passed to SDK

[shorowit]

Enhancement Request

Improve the handling of invalid argument values passed to SDK methods. The current behavior is arguably dangerous and can result in software bugs and incorrect simulation results that developers/users are likely not even aware of.

Detailed Description

Currently when an OpenStudio measure passes an invalid argument value to an SDK method, the method will not apply the change and simply return false. This means that a default value may get used in the EnergyPlus simulation unbeknownst to the user, which, depending on the EnergyPlus input, could case a severe impact to simulation results.

A couple real-world examples where this occurred:

• OpenStudio-HPXML: It was possible to calculate a negative water heater tank loss coefficient and try to assign that to the water heater. (We added error-checking to fix this.) The bug resulted in very high water heating energy use in the simulation results.
• OpenStudio-standards: EnergyPlus changed its fuel type enumerations but an old value is still being used by openstudio-standards. The bug results in the wrong heating fuel type in the simulation results.

I'm sure there are other bugs I'm unaware of. 😱

It is very easy for a developer/user to be blissfully unaware that an invalid value was passed and a default value is being used instead. The only way to currently identify these issues is to check the return value of every SDK method call, but this would make measures' code significantly more complex.

Possible Implementation

Option 1. I would strongly advocate for the SDK raising an exception that would cause the OS measure to fail, so that the measure can be fixed. An on/off switch could be allowed if some developers don't want their measures to break and want to continue living in ignorant bliss.

Option 2. Another option would be to add model.warnings and model.errors methods, similar to the forward translator.

Option 3. One last option would be to allow the invalid arguments to persist in the OS model. Then they could A) get validated once at the end of model creation or B) get passed to EnergyPlus and result in a simulation error, allowing the developer to fix the issue. This might have unintended consequences? But it would also probably improve performance pretty significantly.

[jmarrec]

(Almost) every setter in the OS SDK has the form bool setXXX(...). The point of the bool return value is to let you know whether that worked or not, without raising exceptions (which is good!). I strongly agree with this design decision (though it was done way before I was involved)

When writing a measure you are able to capture that return value and throw if you'd like. I understand that doing that for every call might be tedious, but you can perhaps get creative?

I carry this in my ~/.pryrc to mimic the google test assertions (so I can paste C++ unit test code into ruby more easily), you can use it in a measure if you'd like. It's crude but does the job

class AssertionError < StandardError
  def initialize(msg="Assertion Error")
    super
  end
end

def ASSERT_TRUE(value)
  if value.class.to_s[/OpenStudio/]
    if value.class.to_s[/Optional/]
      if value.empty?
        raise AssertionError, "#{value}: Optional not initialized"
      end
    else
      raise "Couldn't understand #{value.briefDescription()} in a boolean context"
    end
  elsif !value
    raise AssertionError, "#{value} != true"
  end
end

m = Model.new
c = CoilHeatingGas.new(m)
ASSERT_TRUE(c.setFuelType('BADVALUE'))
AssertionError: false != true
from /home/julien/.pryrc:285:in `ASSERT_TRUE'

I'm not sure where a switch like this would live in the C++ code.

[jmarrec]

This is slightly better, as it will give a traceback to the line of the call (instead of the ASSERT_TRUE function scope):

def raise_if_setter_failed(&block)
  block.call.tap do |value|
    if !value
      puts "Failed to set value"
      raise "Failed to set value"
    end
  end
end


m = Model.new
c = CoilHeatingGas.new(m)
raise_if_setter_failed { c.setFuelType('BADVALUE') }

[jmarrec]

I'm not sure where a switch like this would live in the C++ code.

I suppose we could carry a bool m_raiseIfSetterFailed but I am strongly against this idea. That means one extra condition to check for a very low level function (setString, which is called ALL the time), which is likely to negatively affect performance for everyone involved regardless of whether they care or not.

Bottom line: IMHO this is a won't fix / works as expected issue and should be closed. Measure writers are responsible to check for the validity if they care about it, and there are creative ways to avoid doing dozens of if xx.setYYY then raise.

[shorowit]

I can't disagree more.

Sorry, but those are really poor solutions. Asking measure writers to "get creative" and update thousands of lines of code with some kind of pattern like that is not practical and will make the code look ridiculous.

[shorowit]

Just came across another example where a measure was providing incorrect values and so defaults were being unused. The developer was unaware of this bug in the code.

[tijcolem]

Could be added to #4458 if Option 3B is enabled.