Fix #11050 - Schedule initialization error in HighTempRadiantSystem #11053

amirroth · 2025-05-11T19:20:53Z

Pull request overview

Fixes Schedule initialization error in HighTempRadiantSystem #11050

One line fix. Should be self-explanatory.

amirroth · 2025-05-11T19:23:13Z

src/EnergyPlus/HighTempRadiantSystem.cc

            }

            if (state.dataIPShortCut->lAlphaFieldBlanks(7)) {
+                highTempRadSys.setptSched = Sched::GetScheduleAlwaysOff(state);


Empty field means "AlwaysOff" schedule, so initialize with pointer to "AlwaysOff" schedule object. Not sure how "AlwaysOff" is a setpoint exactly, but that was the previous behavior. There could be a few more of these stragglers and this is the fix for all of them.

Seems like it's dependent on the "Temperature control type" field.

MeanAirTemperature MeanRadiantTemperature OperativeTemperature MeanAirTemperatureSetpoint MeanRadiantTemperatureSetpoint OperativeTemperatureSetpoint

One could argue that it should probably fatal out if the ControlType is a Setpoint one and the schedule is missing, but I think it just makes the HighTempRadiant to never do anything,

EnergyPlus/src/EnergyPlus/HighTempRadiantSystem.cc

Line 915 in 09a25d0

if (ZoneTemp < (SetPtTemp - TempConvToler)) {

If one can argue that it should fatal out than I will make it fatal out. If one can argue that this should be a warning that the system will be disabled then I will do that. I would like to handle this the "right" way. Silently disabling this system if the schedule is empty does not seem like the "right" way.

If one can argue that it should fatal out than I will make it fatal out. If one can argue that this should be a warning that the system will be disabled then I will do that. I would like to handle this the "right" way. Silently disabling this system if the schedule is empty does not seem like the "right" way.

@RKStrand ?

One could start by checking what's really happening if this is blank. https://github.com/NREL/EnergyPlusDevSupport/commit/a18bcf32f0b43ff92a9ebc39169c4e3c30afd854

One could perhaps start by creating four MCVEs:

TEMP_CTRL_TYPES = ['Operative', 'OperativeSetpoint']

HAS_SCH = [True, False]

Then run them, then parse the end use by fuel table.

Something like this: https://github.com/NREL/EnergyPlusDevSupport/blob/a18bcf32f0b43ff92a9ebc39169c4e3c30afd854/DefectFiles/11000s/11050/One_could_start_by_checking_what_is_happening_now.ipynb

The schedule is also used if it's not a SetpointControl temperature control type.

I/O Ref states this:

Field: Heating Setpoint Temperature Schedule Name[LINK]

This field specifies the heating setpoint or control temperature for the radiant system in degrees Celsius.

EnergyPlus/src/EnergyPlus/HighTempRadiantSystem.cc

Line 804 in 09a25d0

Real64 SetPtTemp = thisHTR.setptSched->getCurrentVal();

One could draw the conclusion that the E+ IDD should probably just make this schedule \required-field.

I just noticed this too when trying to change the error condition. An empty schedule essentially disables the whole system silently. That's not the right thing to do.

I redid the analysis by checking all possible combinations

TEMP_CTRL_TYPES = [ 'MeanAirTemperature', 'MeanRadiantTemperature', 'OperativeTemperature', 'MeanAirTemperatureSetpoint', 'MeanRadiantTemperatureSetpoint', 'OperativeTemperatureSetpoint' ] HAS_SCH = [True, False]

TemperatureControlType (A6) is also not a required field. And when it is left empty, there are is no error message but there are asserts.

jmarrec · 2025-05-12T09:15:40Z

src/EnergyPlus/HighTempRadiantSystem.cc

            }

            if (state.dataIPShortCut->lAlphaFieldBlanks(7)) {
+                highTempRadSys.setptSched = Sched::GetScheduleAlwaysOff(state);


Seems like it's dependent on the "Temperature control type" field.

MeanAirTemperature MeanRadiantTemperature OperativeTemperature MeanAirTemperatureSetpoint MeanRadiantTemperatureSetpoint OperativeTemperatureSetpoint

One could argue that it should probably fatal out if the ControlType is a Setpoint one and the schedule is missing, but I think it just makes the HighTempRadiant to never do anything,

EnergyPlus/src/EnergyPlus/HighTempRadiantSystem.cc

Line 915 in 09a25d0

if (ZoneTemp < (SetPtTemp - TempConvToler)) {

RKStrand · 2025-05-13T13:37:24Z

Sorry, all, that I have not been able to keep up on this thread. I'm not sure why the code is currently the way that it is. It's been around for a while (I think this was implemented in the early 2000s), and I honestly can't remember why I would have done it this way. If there is not temperature control type, this is a pointless system that won't do anything. Same goes for the lack of a control temperature schedule. The only reason I can think of to allow the user to do this would be if someone is doing a bunch of iterations and wants an easy way to lock out this system without having to delete the whole thing. I'm not sure that's a good enough reason to allow this unless @mjwitte thinks there is merit to that. I think I would vote for requiring both the control type and the schedule with a severe followed by a fatal error if one or both are missing.

mjwitte · 2025-05-13T14:10:30Z

Temperature control type has a default, so that's covered.

   A6 , \field Temperature Control Type
        \default OperativeTemperature

But the Heating Setpoint Temperature Schedule should be required. One could argue that this should have a transition rule, but it's a pointless simulation without one. So, we could emit a transition warning about it or just let it fly by.

BTW this idd note is wrong:

   A7 , \field Heating Setpoint Temperature Schedule Name
        \note This setpoint is an "operative temperature" setpoint

Should be?

        \note This setpoint is mean air temperature, radiant temperature or operative temperature depending on the Temperature Control Type.

…hange input processing and error handling to match

amirroth · 2025-05-13T15:21:38Z

Ok, I made the changes suggested by @mjwitte and @RKStrand. @jmarrec, your file is now going to fatal out at the epJSON parser. If you still want to run it, one way to do that is to make the Setpt Schedule Name Constant-0.0 (that's the name of the builtin "AlwaysOff" schedule. Is this satisfactory?

Myoldmopar · 2025-05-14T13:23:45Z

I'll get the branch fixed up and pushed. @mjwitte are you satisfied with this change now?

rraustad · 2025-05-14T13:48:48Z

src/EnergyPlus/HighTempRadiantSystem.cc

-            highTempRadSys.ControlType = static_cast<RadControlType>(getEnumValue(radControlTypeNamesUC, state.dataIPShortCut->cAlphaArgs(6)));
+            if (state.dataIPShortCut->lAlphaFieldBlanks(6)) {
+                ShowWarningEmptyField(state, eoh, state.dataIPShortCut->cAlphaFieldNames(6), "OperativeTemperature");
+                highTempRadSys.ControlType = RadControlType::OperativeControl;


A6 will never be blank since it has a default. Old line 443 is all that is needed.

A6 , \field Temperature Control Type \note Temperature type used to control unit \type choice \key MeanAirTemperature \key MeanRadiantTemperature \key OperativeTemperature \key MeanAirTemperatureSetpoint \key MeanRadiantTemperatureSetpoint \key OperativeTemperatureSetpoint \default OperativeTemperature

rraustad · 2025-05-14T13:50:44Z

src/EnergyPlus/HighTempRadiantSystem.cc

+                ErrorsFound = true;
            } else if ((highTempRadSys.setptSched = Sched::GetSchedule(state, state.dataIPShortCut->cAlphaArgs(7))) == nullptr) {
                ShowSevereItemNotFound(state, eoh, state.dataIPShortCut->cAlphaFieldNames(7), state.dataIPShortCut->cAlphaArgs(7));
                ErrorsFound = true;


A7 is now a required field so it can't be blank (I think) but it can have an invalid name, so 467 should be the only conditional?

mjwitte · 2025-05-14T14:05:56Z

I'll get the branch fixed up and pushed. @mjwitte are you satisfied with this change now?

What @rraustad said.

Myoldmopar · 2025-05-14T19:53:26Z

OK, this was blessed as-is, with an understanding that maybe we can clean up some unnecessary error checking later. @rraustad suggested checking the unit/integration test coverage to find get-input lines that aren't covered by any test, and use that as a starting point for detecting unnecessary checks. Seems like as reasonable a jumping off point as any.

Merging this, thanks @amirroth and reviewers.

Fix NREL#11050

One line fix seems to work

09a25d0

amirroth commented May 11, 2025

View reviewed changes

amirroth requested a review from jmarrec May 11, 2025 19:23

amirroth assigned jmarrec May 11, 2025

amirroth added the Defect Includes code to repair a defect in EnergyPlus label May 11, 2025

amirroth added this to the EnergyPlus 25.2 milestone May 11, 2025

jmarrec approved these changes May 12, 2025

View reviewed changes

Make HighTempRadiantSystem setpt schedule name a required field and c…

1c42fa8

…hange input processing and error handling to match

Clang format

cee1dc0

rraustad reviewed May 14, 2025

View reviewed changes

Myoldmopar merged commit 542707a into develop May 14, 2025
9 checks passed

Myoldmopar deleted the Fix11050 branch May 14, 2025 19:53

jmarrec pushed a commit to jmarrec/EnergyPlus that referenced this pull request May 26, 2025

Merge pull request NREL#11053 from NREL/Fix11050

6fee28e

Fix NREL#11050

jmarrec changed the title ~~Fix #11050~~ Fix #11050 - Schedule initialization error in HighTempRadiantSystem May 26, 2025

jmarrec mentioned this pull request May 26, 2025

Fix #4219 - Fix #4628 - DesignSpecificationOutdoorAir (use a DSOA:SpaceList if appropriate insead of using first dsoa found) and ControllerMechanicalVentilation upgrades NREL/OpenStudio#5384

Merged

21 tasks

Fix #11050 - Schedule initialization error in HighTempRadiantSystem #11053

Fix #11050 - Schedule initialization error in HighTempRadiantSystem #11053

Uh oh!

Conversation

amirroth commented May 11, 2025

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RKStrand commented May 13, 2025

Uh oh!

mjwitte commented May 13, 2025

Uh oh!

amirroth commented May 13, 2025

Uh oh!

Myoldmopar commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjwitte commented May 14, 2025

Uh oh!

Myoldmopar commented May 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jmarrec May 13, 2025 •

edited

Loading