Followup VRF warning about solveRoot of Te at small load condition by yujiex · Pull Request #11000 · NatLabRockies/EnergyPlus

yujiex · 2025-03-20T21:30:26Z

Pull request overview

Addressing follow-up discussions in PR Followup on the assertion failure solving Te at low load condition in VRFFluidTCtrl model #10994

Description of the purpose of this PR

Pull Request Author

Reviewer

github-actions · 2025-03-20T22:28:54Z

⚠️ Regressions detected on macos-14 for commit `84c530b`

Regression Summary

EIO: 1
ERR: 4
ESO Big Diffs: 4
Table Big Diffs: 1
MTR Small Diffs: 2
Table Small Diffs: 2

rraustad · 2025-03-21T01:21:35Z

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

-                        } else {
-                            SmallLoadTe = MinOutdoorUnitTe;
-                        }
+                                   T_suction + 6); // SmallLoadTe is the updated Te'


Why is this SolveRoot call, lines 13960 and 13961, still here? It doesn't do anything.

If you are going to execute line 13968 anyway, then you may as well make this an if () { } else { } to eliminate the extra equivalence.

I would still like to see a warning for SolFla == -2 and let the code coverage tell us it's never used than to possibly miss a coding mistake. At the very least put an assert there so a developer might stumble across an issue.

What happened to the recurring warning?

@rraustad Thanks for the comments.
for 1. I think might have forgot to change it back. Now the upper bound is changed back to T_suction.
2. I changed it to if else logic
3. I added back the warnings for the SolFla = -2 case
4. Also added back recurring warnings in the SolFla = -2

rraustad · 2025-03-21T01:21:55Z

src/EnergyPlus/HVACVariableRefrigerantFlow.hh

+        int LowLoadTeError2PosTsuc = 0;
+        int LowLoadTeError2PosTsucIndex = 0; // warning message index
+        int LowLoadTeError2PosOUTe = 0;
+        int LowLoadTeError2PosOUTeIndex = 0; // warning message index


Are you planning to use these?

yes, added back the warnings

github-actions · 2025-03-21T19:22:20Z

⚠️ Regressions detected on macos-14 for commit `cf8fe9f`

Regression Summary

EIO: 1
ERR: 4
ESO Big Diffs: 4
Table Big Diffs: 1
MTR Small Diffs: 2
Table Small Diffs: 2

github-actions · 2025-03-21T22:36:29Z

⚠️ Regressions detected on macos-14 for commit `6b4737a`

Regression Summary

EIO: 1
ERR: 4
ESO Big Diffs: 4
Table Big Diffs: 1
MTR Small Diffs: 2
Table Small Diffs: 2

nrel-bot-2 · 2025-05-09T00:42:14Z

@Myoldmopar @yujiex @Myoldmopar it has been 29 days since this pull request was last updated.

nrel-bot-2c · 2025-06-12T16:09:21Z

@Myoldmopar @yujiex @Myoldmopar it has been 35 days since this pull request was last updated.

github-actions · 2025-07-02T17:57:03Z

⚠️ Regressions detected on macos-14 for commit `74ece32`

Regression Summary

EIO: 1
ERR: 4
ESO Big Diffs: 4
Table Big Diffs: 1
MTR Small Diffs: 2
Table Small Diffs: 2

This reverts commit 3399351.

github-actions · 2025-07-02T19:26:25Z

⚠️ Regressions detected on macos-14 for commit `c45c685`

Regression Summary

EIO: 1
ERR: 4
ESO Big Diffs: 4
Table Big Diffs: 1
MTR Small Diffs: 2
Table Small Diffs: 2

github-actions · 2025-07-16T17:57:28Z

⚠️ Regressions detected on macos-14 for commit `17bf038`

Regression Summary

EIO: 1
ERR: 4
ESO Big Diffs: 4
Table Big Diffs: 1
MTR Small Diffs: 2
Table Small Diffs: 2

github-actions · 2025-08-27T18:35:05Z

⚠️ Regressions detected on macos-14 for commit `423492e`

Regression Summary

EIO: 1
ERR: 4
ESO Big Diffs: 4
Table Big Diffs: 1
MTR Small Diffs: 2
Table Small Diffs: 2

nrel-bot-2 · 2025-09-24T16:01:13Z

@Myoldmopar @yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

rraustad · 2025-10-10T22:48:11Z

src/EnergyPlus/HVACVariableRefrigerantFlow.cc


                    General::SolveRoot(state, 1.0e-3, MaxIter, SolFla, SmallLoadTe, f, MinOutdoorUnitTe,
                                       T_suction); // SmallLoadTe is the updated Te'
-                    if (SolFla == -1) {


Line 14219 was moved to new line 14232, right? If so, the call to SolveRoot at 14219 can be deleted.

rraustad · 2025-10-10T22:51:28Z

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

+                                    SmallLoadTe);
+                            } else {
+                                // demand > capacity at both endpoints of the Te range, take the end point x where f(x) is closer to zero
+                                if (f_xmin > f_xmax) { // f(T_suction > 0, not equal as SolFla will not be -2


Is this comment SolFla will not be -2 correct? because this code is in the } else if (SolFla == -2) { block.

rraustad · 2025-10-10T22:53:24Z

@yujiex this is about ready. Just take a look at these last comments.

nrel-bot-2c · 2025-11-08T00:48:02Z

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

nrel-bot-2 · 2025-12-06T00:38:43Z

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

nrel-bot-2c · 2026-01-03T00:46:10Z

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

…s://github.com/NREL/EnergyPlus into followupVRFwarning

rraustad · 2026-01-22T20:03:38Z

Testing a comment. I can see the repo but cannot push this branch.

rraustad

This looks good now.

rraustad · 2026-01-23T04:32:56Z

I did look at the defect file for issues (I tracked back through PRs for a defect file). I am not sure what COP should result from this "VRF configuration" but I don't see any oddities, i.e., step functions in performance because of these changes (except for a cooling COP > 8, that does seem rather high but out of scope). High cooling COP could be a follow up, if necessary, and does not relate to these changes.

d

11000-US+SF+CZ5B+hp+slab+IECC_2021_VRFPhysics_v2_hrdsize_V2420.idf.txt

mitchute · 2026-01-23T21:19:02Z

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

+                            }
+                            ShowRecurringWarningErrorAtEnd(
+                                state,
+                                "Low load calculation Te solution not found as load is smaller than min-speed capacity for the whole range",


Maybe it's just me, but I don't understand what the warning is trying to convey. Is "Te" something you'd expect the user to understand?

This warning seems more like a diagnostic message for developers, rather than a user-friendly explanation. We might consider making it more user-oriented.

dareumnam · 2026-01-23T22:02:05Z

This looks good to me as-is, and the extra handling for SolFla == -2 definitely makes the low-load behavior easier to understand.
As a small suggestion for potential cleanup later on, I think it might be helpful to explicitly clarify the sign convention of the residual f(Te), whether it’s defined as (demand - capacity) or (capacity - demand).
Related to that, in the SolFla == -2 fallback cases where both endpoints have the same sign, I think it could be worth considering picking the endpoint with the smallest absolute residual (closest to zero). The current endpoint-based choice seems fine, but this is just a thought that might make the logic a bit more explicit.
That said, I’m happy with the current implementation. Thanks so much for the follow-up PR, @yujiex, and for the final touch-up and results, @rraustad. I think it is ready to be merged. @mitchute

mitchute · 2026-01-23T22:09:23Z

Thanks all. We'll merge this. If in hindsight there's more clarity that can be added to those messages, please considers submitting a follow-up.

Yujie Xu added 2 commits March 20, 2025 14:14

separate warning for -1 and -2, report SmallLoadTe range in warning

1d3ab1c

check sign of end points before solving root

c77f5d1

yujiex added the Defect Includes code to repair a defect in EnergyPlus label Mar 20, 2025

yujiex self-assigned this Mar 20, 2025

yujiex requested a review from rraustad March 20, 2025 21:30

yujiex mentioned this pull request Mar 20, 2025

Followup on the assertion failure solving Te at low load condition in VRFFluidTCtrl model #10994

Merged

20 tasks

rraustad reviewed Mar 21, 2025

View reviewed changes

add back warnings, keep old upper bound SolveRoot

51cb011

Fix unit test

4cff147

Myoldmopar self-assigned this Apr 9, 2025

Yujie Xu added 3 commits July 1, 2025 16:50

Merge remote-tracking branch 'origin/develop' into followupVRFwarning

dc754c8

fix bracket issue from merging

7d4fa9c

temp mod feenableexcept so apple arm can build

3399351

Yujie Xu added 2 commits July 2, 2025 11:25

Revert "temp mod feenableexcept so apple arm can build"

67b3cf3

This reverts commit 3399351.

apply clang-format

bbf4214

Merge remote-tracking branch 'origin/develop' into followupVRFwarning

821c29e

yujiex added this to the EnergyPlus 25.2 milestone Aug 7, 2025

Merge remote-tracking branch 'origin/develop' into followupVRFwarning

7bfd8b2

dareumnam self-assigned this Sep 24, 2025

dareumnam unassigned Myoldmopar and dareumnam Sep 24, 2025

dareumnam self-requested a review September 24, 2025 18:29

rraustad reviewed Oct 10, 2025

View reviewed changes

rraustad added 2 commits January 21, 2026 16:55

Update new SolveRoot failure code, and Merge branch 'develop' of http…

8940c69

…s://github.com/NREL/EnergyPlus into followupVRFwarning

Remove unused and redundant code

125bd13

rraustad and others added 2 commits January 22, 2026 22:03

Simple comment update on cloned repo to test workflow

964e38d

Test yml update

cf8ea42

rraustad approved these changes Jan 23, 2026

View reviewed changes

dareumnam modified the milestones: EnergyPlus 25.2, EnergyPlus 26.1 Bug Fix Freeze Jan 23, 2026

mitchute reviewed Jan 23, 2026

View reviewed changes

mitchute merged commit 54a3f66 into develop Jan 23, 2026
11 checks passed

mitchute deleted the followupVRFwarning branch January 23, 2026 22:09

Conversation

yujiex commented Mar 20, 2025

Pull request overview

Description of the purpose of this PR

Pull Request Author

Reviewer

Uh oh!

github-actions bot commented Mar 20, 2025

⚠️ Regressions detected on macos-14 for commit 84c530b

Uh oh!

rraustad Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

yujiex Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

rraustad Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

yujiex Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 21, 2025

⚠️ Regressions detected on macos-14 for commit cf8fe9f

Uh oh!

github-actions bot commented Mar 21, 2025

⚠️ Regressions detected on macos-14 for commit 6b4737a

Uh oh!

nrel-bot-2 commented May 9, 2025

Uh oh!

nrel-bot-2c commented Jun 12, 2025

Uh oh!

github-actions bot commented Jul 2, 2025

⚠️ Regressions detected on macos-14 for commit 74ece32

Uh oh!

github-actions bot commented Jul 2, 2025

⚠️ Regressions detected on macos-14 for commit c45c685

Uh oh!

github-actions bot commented Jul 16, 2025

⚠️ Regressions detected on macos-14 for commit 17bf038

Uh oh!

github-actions bot commented Aug 27, 2025

⚠️ Regressions detected on macos-14 for commit 423492e

Uh oh!

nrel-bot-2 commented Sep 24, 2025

Uh oh!

rraustad Oct 10, 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!

rraustad Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

rraustad commented Oct 10, 2025

Uh oh!

nrel-bot-2c commented Nov 8, 2025

Uh oh!

nrel-bot-2 commented Dec 6, 2025

Uh oh!

nrel-bot-2c commented Jan 3, 2026

Uh oh!

rraustad commented Jan 22, 2026

Uh oh!

rraustad left a comment

Choose a reason for hiding this comment

Uh oh!

rraustad commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitchute Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

dareumnam Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

dareumnam commented Jan 23, 2026

Uh oh!

mitchute commented Jan 23, 2026

Uh oh!

Uh oh!

⚠️ Regressions detected on macos-14 for commit `84c530b`

⚠️ Regressions detected on macos-14 for commit `cf8fe9f`

⚠️ Regressions detected on macos-14 for commit `6b4737a`

⚠️ Regressions detected on macos-14 for commit `74ece32`

⚠️ Regressions detected on macos-14 for commit `c45c685`

⚠️ Regressions detected on macos-14 for commit `17bf038`

⚠️ Regressions detected on macos-14 for commit `423492e`

rraustad Oct 10, 2025 •

edited

Loading

rraustad commented Jan 23, 2026 •

edited

Loading