ibpsa / project1-boptest

Building Optimization Performance Tests
Other
109 stars 70 forks source link

Enable getting values of all inputs #364

Closed dhblum closed 2 years ago

dhblum commented 3 years ago

Currently, in order be able to get the value of an input, the test case developer needs to explicitly add a read block following an overwrite block. There are some cases where this happens, e.g. in bestest_hydronic_heat_pump, and some cases where this doesn't, e.g. in multizone_residential_hydronic. In the latter, particularly look at the model ConHeaZon (figure below), which is the general implementation of the zone temperature controllers. To a user reading the measurements, they can retrieve the value of the control signal leaving the PI controller as reaActHea, but they can't retrieve the value of the controller set point leaving oveTSetHea. Thus, it is difficult to know what the baseline control is.

Screen Shot 2021-10-06 at 1 17 41 PM

My suggestion is to add functionality to the parser.py such that for every overwrite block, there is a corresponding wrapper output variable added. I believe it is reasonable to assume for a BAS that one would be able to read the value of such control signals. The form would be as follows:

Once functionality added, I recommend removing the read blocks in the existing emulators that are used for the reading of control signals.

@SenHuang19 I know this is something that you've brought up before, but in response to https://github.com/ibpsa/project1-boptest/issues/359 perhaps should move up the priority list.

Feel free to provide any feedback. FYI also @JavierArroyoBastida.

javiarrobas commented 3 years ago

I think this is a necessary issue. Thanks @dhblum for bringing it up.

As for the implementation, wouldn't it be better to directly add a read block within the Overwrite Modelica class? so that parser.py does not need to be modified. I agree that the existing read blocks for reading control signals should be removed when implementing either option.

dhblum commented 3 years ago

True, that would relieve modification of parser.py. Although, then we need to PR Modelica IBPSA and have Buildings and IDEAS sync to propagate into test case models. Not necessarily saying this is worse.

javiarrobas commented 3 years ago

I like having the process of tagging decoupled from the process of preparing the wrapped model. However, I have no strong opinion and I think both options would work fine.

dhblum commented 3 years ago

The process of tagging is what preparing the wrapped model is about I think though. It is actually a small edit to enable this, see https://github.com/ibpsa/project1-boptest/commit/7b543edc72e5c8c95c49c848b42cb01e454c627a. One benefit of this is I think in the future if we want to otherwise somehow distinguish an output that is a "current value" of an input, rather than a sensor output, through tags or changes to the API. This way does not require any more Modelica parsing to first figure out if a given read block came from within a write block or not.

javiarrobas commented 3 years ago

Seems like a good enough reason to go in that direction. Also, looking at the commit it looks like parser.py doesn't need too many edits that way.

dhblum commented 2 years ago

Closed by https://github.com/ibpsa/project1-boptest/pull/377.

dhblum commented 2 years ago

Note conversation of what was implemented is continued in the PR: https://github.com/ibpsa/project1-boptest/issues/364. I think the documentation (see section Parsing and FMU Export) needs to be updated for what was actually implemented in that PR.