Addresses #4223, wrap the new ElectricLoadCenter:Storage:LiIonNMCBattery object in the OS SDK

[joseph-robertson]

Pull request overview

• Fixes E+ 9.5.0: Wrap the new ElectricLoadCenter:Storage:LiIonNMCBattery object in the OS SDK #4223

Please read OpenStudio Pull Requests to better understand the OpenStudio Pull Request protocol.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

• 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)
• Checked behavior in OpenStudioApplication, adjusted policies as needed (src/openstudio_lib/library/OpenStudioPolicy.xml)
• 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

[jmarrec]

LGTM, I suggest merging @kbenne

[joseph-robertson]

@jmarrec @kbenne Not quite ready to merge yet; still waiting for feedback on constructor(s) from @nmerket.

[nmerket]

@joseph-robertson I made these changes to get the fields we need to construct the object before doing so. Because they're required, I'm no longer doing the if(i) ... before setting it because it needs to be there. Is using OS_ASSERT a good way to do that here, or do I need to have it return a more gentle error message?

[joseph-robertson]

I'd say follow this as an example: https://github.com/NREL/OpenStudio/blob/develop/src/energyplus/ReverseTranslator/ReverseTranslateSurfaceControlMovableInsulation.cpp. Both surface and material are passed into the ctor.

[nmerket]

I still want to try to come up with an additional alternative constructor and possibly a setter or two that takes more user-friendly inputs like "target battery capacity [kwh]" and generates some of the other inputs like series and parallel from that. However, if we need to merge this sooner, I'm fine with this the way it is and I'll do that in a later version or in our measure code.

[nmerket]

I changed the constructor to require these additional fields.

[jmarrec]

I'm not a big fan of this change. If we can provide reasonable defaults (I think we can, just pick whatever is in your E+ example file) I'd prefer to have a ElectricLoadCenterStorageLiIonNMCBattery(const Model& model) ctor.

You can have 2 constructors if you want this explicit one. And you could still throw in the ReverseTranslator if the IDF doesn't have one of these fields if you like (or issue a Warn/Error, and let the default pass in)

(This isn't "out the blue", we've had lengthy discussions about this in the past).

[nmerket]

@joseph-robertson Do you know what's going wrong with the windows incremental build?

[joseph-robertson]

@nmerket I don't think it's related to anything you're doing. Question for @tijcolem.

[jmarrec]

@joseph-robertson Do you know what's going wrong with the windows incremental build?

MSVC is throwing because of your RT:

    OptionalInt nSeries;
    nSeries = workspaceObject.getInt(ElectricLoadCenter_Storage_LiIonNMCBatteryFields::NumberofCellsinSeries);
    if (not nSeries) {     //// <============= THIS, should be `!nSeries` instead

(not is an alternative token for logical operator !, which MSVC does not define by default unlike clang/GCC: see https://stackoverflow.com/questions/24414124/why-does-vs-not-define-the-alternative-tokens-for-logical-operators)

I fixed this in b194c95 and did a little change while I was at in in 24d3eb2

[jmarrec]

Ok, now Windows reports a failure that you can ignore, it's a know ruby test that's failing

The following tests FAILED:
	3000 - RubyTest-Date_Test-ymd_constructor (Failed)

[joseph-robertson]

@nmerket @jmarrec So what's the decision here? To merge as it is now? Or to provide alternative ctor(s)?

[jmarrec]

I would personally like an alternative, model only, ctor. I'm fine with the rest. @kbenne thoughts?

[kbenne]

I would personally like an alternative, model only, ctor. I'm fine with the rest. @kbenne thoughts?

I also prefer that ModelObject constructors offer one overload that requires only a model argument. Nothing wrong with other more explicit overloads, but I feel like the simple construction with one argument makes the API more approachable.

[nmerket]

@jmarrec @kbenne I intentionally removed the "model only" constructor because I felt like some fields should be provided by the user. The additional constructor I am thinking of adding is one that will calculate the number of series and parallel based on a desired capacity. I'm worried that a user will just use the default constructor and assume that it's a good model, when it's not. Maybe I'm not understanding the paradigm of how the OpenStudio API should work.

[nmerket]

Upon further consideration, I think I'm going to wait until the next release to add another constructor with the capacity that calculates the series and parallel. There's still some logic we need to figure out on what should be defaulted and how that will work in practice.

As far as I'm concerned, I'm happy with this the way it is to merge, so don't wait on me. I'm headed out of town next week and won't be able to work on this, so if you feel very strongly that it needs a model only constructor, please go ahead and put it back in.

Thanks for being patient with me on this.

[jmarrec]

@jmarrec @kbenne I intentionally removed the "model only" constructor because I felt like some fields should be provided by the user. The additional constructor I am thinking of adding is one that will calculate the number of series and parallel based on a desired capacity. I'm worried that a user will just use the default constructor and assume that it's a good model, when it's not. Maybe I'm not understanding the paradigm of how the OpenStudio API should work.

@nmerket I totally understand your initial point of view on this, but I'll try to provide some insight as to why I think differently. I think providing a Ctor that takes the required argument(s) is fine and helps drive the point that they should be provided. But I still want a Ctor that takes Model& only if we can (the cases where we can't are very few). The only arguments that would be valid to pass in - IMHO - are required objects (Curves, HVACComponents, etc). And in most cases I'd still want to have one that takes these, and one that takes Model& only and instantiates the rest so we have:

1. An easy way to use the object and inspect it by providing Model& only
2. An advanced way of passing everything required to avoid duplicating objects (especially for ResourceObjects, such as Curves) if you are concerned about speed/performance, file size, or avoid repeating yourself

The reason is because it's much harder to use the API if you don't. When I need to write a measure or some code, I jump into an interactive ruby prompt (via pry, which is irb but with better tab completion). Most of the time, I would try to construct the object I'm interested it by just providing Model to the class Ctor, and most of the time it works. If not, I can generally figure it out. Once I have the object, I can print it, inspect its methods, etc, so I know how to use the object without any external resources. Having to go look up SDK documentation is annoying (mind that in most cases you also are already looking at the E+ I/O ref...)

In your specific case, you're trying to require numeric fields. Most objects in the API have double/integer/choice fields that must be tweaked in order to produce realistic results (or just because they are marked \required-field in the IDD). If we suddenly made Ctors that take in all of these, most Ctors would be 5 to 10 arguments long and the API would be a nightmare to use.
It's the user's responsibility to tweak these, or embrace the Garbage-In Garbage-Out (GIGO) principle.

---

Details

Some Demonstration on my API usage, click to expand

Most of the time, I can construct an object

[1] julien(main)> include OpenStudio::Model
=> Object
[2] julien(main)> m = Model.new
=> #<OpenStudio::Model::Model:0x000055b4845e08d8
 @__swigtype__="_p_openstudio__model__Model">
[3] julien(main)> b = BoilerHotWater.new(m)
=> #<OpenStudio::Model::BoilerHotWater:0x000055b4840b3450

Sometimes it doesn't work, but I can trick ruby (well, SWIG to be exact) into giving me more info by passing bogus args, and I get enough info to go on (eg: below, from domain knowledge and argument type, I know it's the AvailabilitySchedule that's required).

[6] julien(main)> atu = AirTerminalSingleDuctConstantVolumeNoReheat.new(m)
ArgumentError: wrong # of arguments(1 for 2)
from (pry):6:in `initialize'

# Pass bogus arg
[7] julien(main)> atu = AirTerminalSingleDuctConstantVolumeNoReheat.new(m, "")
TypeError: Expected argument 1 of type openstudio::model::Schedule &, but got String ""
	in SWIG method 'AirTerminalSingleDuctConstantVolumeNoReheat'
from (pry):7:in `initialize'

Now, what happens when you provide a Ctor that needs a lot of arguments, and with the same type for different args? Well, there is no way other than to go to the SDK documentation to look it up, which is clumsy and makes you lose time.

[8] julien(main)> ptac = ZoneHVACPackagedTerminalAirConditioner.new(m)
ArgumentError: wrong # of arguments(1 for 5)
from (pry):8:in `initialize'
[9] julien(main)> ptac = ZoneHVACPackagedTerminalAirConditioner.new(m, "", "", "", "")
TypeError: Expected argument 1 of type openstudio::model::Schedule &, but got String ""
	in SWIG method 'ZoneHVACPackagedTerminalAirConditioner'
from (pry):9:in `initialize'
[10] julien(main)> ptac = ZoneHVACPackagedTerminalAirConditioner.new(m, m.alwaysOnDiscreteSchedule, "", "", "")
TypeError: Expected argument 2 of type openstudio::model::HVACComponent &, but got String ""
	in SWIG method 'ZoneHVACPackagedTerminalAirConditioner'

# At this point I know it's expecting either the Fan, the Heating COil or the Cooling coil, which one is it?!

I need several clicks to get here: https://openstudio-sdk-documentation.s3.amazonaws.com/cpp/OpenStudio-3.1.0-doc/model/html/classopenstudio_1_1model_1_1_zone_h_v_a_c_packaged_terminal_air_conditioner.html and even the signature isn't enough, I need an extra click until I see the arguments names

Once I have the object, I can inspect it, look at its methods, etc

[13] julien(main)> puts atu
OS:AirTerminal:SingleDuct:ConstantVolume:NoReheat,
  {d12f35b0-b126-4a67-a928-d5e5e5eb8da8}, !- Handle
  Air Terminal Single Duct Constant Volume No Reheat 1, !- Name
  {b22711ff-fbcd-4967-8fe1-70b8c804d423}, !- Availability Schedule Name
  ,                                       !- Air Inlet Node Name
  ,                                       !- Air Outlet Node Name
  AutoSize;                               !- Maximum Air Flow Rate {m3/s}

=> nil

This is OpenStudio, so most of the time by looking at a field I can infer its corresponding getter/setter. Maximum Air Flow Rate => I expect the getter to be maximumAirFlowRate() and the setter to be setMaximumAirFlowRate. I can also use the methods function, chain it with grep and a regex, to find all Setters that apply.

[14] julien(main)> atu.methods.grep(/^set/)
=> [:setMaximumAirFlowRate,
 :setAvailabilitySchedule,
 :setParent,
 :setPointer,
 :setFieldComment,
 :setString,
 :setDouble,
 :setQuantity,
 :setComment,
 :setUnsigned,
 :setInt,
 :setName]

[jmarrec]

I put back a Model only ctor in 4096a0b. I left the (unusual in my opinion) ctor that takes numeric field in place though.

[ci-commercialbuildings]

CI Results for 278e228:

• Ubuntu 18.04:

  • OpenStudio-3.1.1-alpha+278e228646-Ubuntu-18.04.deb
  • OpenStudio-3.1.1-alpha+278e228646-Ubuntu-18.04.tar.gz

• Ubuntu 20.04:

  • OpenStudio-3.1.1-alpha+278e228646-Ubuntu-20.04.deb
  • OpenStudio-3.1.1-alpha+278e228646-Ubuntu-20.04.tar.gz

• Darwin:

  • OpenStudio-3.1.1-alpha+278e228646-Darwin.dmg
  • OpenStudio-3.1.1-alpha+278e228646-Darwin.tar.gz

• Windows:

  • OpenStudio-3.1.1-alpha+278e228646-Windows.exe
  • OpenStudio-3.1.1-alpha+278e228646-Windows.tar.gz