Fix debug tests #11076
Fix debug tests #11076
Conversation
|
The WCE one is from the Blind PR. I can probably figure that one out in a clean way. I can also try to figure out the tariff one. I’ll look in the next day or two. |
|
I can see the WCE error but it will take me a few days to figure out how to fix it. |
|
I'm gonna go out on a limb and suggest this was a fluke failure. Since the other two builds on that same machine were happy.... ignoring. |
|
Tariff one is gone. The WCE one is interesting. If I roll the code back to 24.1, I see that that block isn't ever hit. I was investigating thinking "why isn't the variable initialized", but the real question appears to be "why is that block being hit"? |
|
Because in the new code everything is an interpolation. |
|
|
|
So this is throwing some table diffs. Looking at them locally, there are multiple places where the monthly diff is off on the last month. I think this is correct. In the current code it was accessing invalid stuff at that last index. Here's the commit that caused it: 153d412 Specifically, before: for (int jPeriod = 1; jPeriod <= countPeriod; ++jPeriod) {
for (int kMonth = 1; kMonth <= MaxNumMonths; ++kMonth) {
monthVal(kMonth) += tariff.gatherEnergy(kMonth, jPeriod);
}
}and after: for (int kMonth = 1; kMonth <= NumMonths; ++kMonth) {
for (int jPeriod = 1; jPeriod <= (int)Period::Num; ++jPeriod) {
monthVal(kMonth) += tariff.gatherEnergy(kMonth)[jPeriod];
}
}The jPeriod lookup has changed to square bracket c++ vector lookup, but the counter variable is still 1-based. In this branch, it simply does jPeriod-1, which gets the seg fault fixed, but with diffs. I think it's good to go. @amirroth or @mjwitte if you see anything I'm missing, I am happy to continue to push on this, but I think this PR is now done. CI is all clean (after a reboot), this PR turns on array bounds checking on Decent, and also fixes the failures that were encountered in doing that. I'll merge it in a while if I don't hear anything. |
|
Hey @Myoldmopar, I don't know if you saw my comment inline on the code but the correct fix for the jPeriod period problem is not |
|
|
@Myoldmopar Why am not seeing the ShopWithPVandLiIonBattery failure on IntegrationCoverage-RelWithDebInfo here? Edit: nevermind, I see you did a new |
|
@Myoldmopar and @amirroth : quite fascinating. I've tried launching a release on my fork. All builds are good, EXCEPT the mac x86_64, which built fine, but it failed to run the PythonPluginCustomSchedule.idf https://github.com/jmarrec/EnergyPlus/actions/runs/15186401562/job/42711866243#step:14:786 I am grabbing that tar.gz on running a throway github actions runners macos-13 and attaching via tmate(ssh), and I can see it failing by attaching lldb (I logically don't have debug symbols, but I can get the routine where it fails) Locally I can actually reproduce the issue with a debug build on Ubuntu I'm honestly quite puzzled as to why mac x86-64 is the only one that catches that. |
|
I find this change hard to grok. A 1-based fortran array of 0-based std::array, what can go wrong :)
|
| for (int jPeriod = 0; jPeriod < (int)Period::Num; ++jPeriod) { | ||
| monthVal(kMonth) += tariff.gatherEnergy(kMonth)[jPeriod]; |
There was a problem hiding this comment.
@amirroth 's recommendation.
Mine would be to stop using an index based loop, so we'd never run into this problem to begin with.
STL accumulate:
for (int kMonth = 1; kMonth <= NumMonths; ++kMonth) {
const auto& energy = tariff.gatherEnergy(kMonth);
monthVal(kMonth) = std::accumulate(energy.begin(), energy.end(), 0.0);
}Range-based for loop.
for (int kMonth = 1; kMonth <= NumMonths; ++kMonth) {
for (const auto& v: tariff.gatherEnergy(kMonth)) {
monthVal(kMonth) += v;
}
}| for (int kMonth = 1; kMonth <= NumMonths; ++kMonth) { | ||
| for (int jPeriod = 1; jPeriod <= (int)Period::Num; ++jPeriod) { | ||
| if (tariff.gatherDemand(kMonth)[jPeriod - 1] > monthVal(kMonth)) { | ||
| monthVal(kMonth) = tariff.gatherDemand(kMonth)[jPeriod - 1]; | ||
| for (int jPeriod = 0; jPeriod < (int)Period::Num; ++jPeriod) { | ||
| if (tariff.gatherDemand(kMonth)[jPeriod] > monthVal(kMonth)) { | ||
| monthVal(kMonth) = tariff.gatherDemand(kMonth)[jPeriod]; |
There was a problem hiding this comment.
We're traversing twice on monthVal to set the demand, which is unnecessary.
There was a problem hiding this comment.
diff --git a/src/EnergyPlus/EconomicTariff.cc b/src/EnergyPlus/EconomicTariff.cc
index 09c45d7966..c13173e469 100644
--- a/src/EnergyPlus/EconomicTariff.cc
+++ b/src/EnergyPlus/EconomicTariff.cc
@@ -3663,27 +3663,19 @@ void setNativeVariables(EnergyPlusData &state)
// nativeTotalEnergy
monthVal = 0.0;
for (int kMonth = 1; kMonth <= NumMonths; ++kMonth) {
- for (int jPeriod = 0; jPeriod < (int)Period::Num; ++jPeriod) {
- monthVal(kMonth) += tariff.gatherEnergy(kMonth)[jPeriod];
- }
+ const auto &energy = tariff.gatherEnergy(kMonth);
+ monthVal(kMonth) = std::accumulate(energy.begin(), energy.end(), 0.0);
}
s_econ->econVar(tariff.natives[(int)Native::TotalEnergy]).values = monthVal;
+
// nativeTotalDemand
monthVal = -bigNumber;
for (int kMonth = 1; kMonth <= NumMonths; ++kMonth) {
- for (int jPeriod = 0; jPeriod < (int)Period::Num; ++jPeriod) {
- if (tariff.gatherDemand(kMonth)[jPeriod] > monthVal(kMonth)) {
- monthVal(kMonth) = tariff.gatherDemand(kMonth)[jPeriod];
- }
- }
- }
- // if no maximum was set just set to zero
- for (int kMonth = 1; kMonth <= NumMonths; ++kMonth) {
- if (monthVal(kMonth) == -bigNumber) {
- monthVal(kMonth) = 0.0;
- }
+ const auto &demand = tariff.gatherDemand(kMonth);
+ monthVal(kMonth) = *std::max_element(demand.begin(), demand.end());
}
s_econ->econVar(tariff.natives[(int)Native::TotalDemand]).values = monthVal;
+
for (int kMonth = 1; kMonth <= NumMonths; ++kMonth) {
// nativePeakEnergy
s_econ->econVar(tariff.natives[(int)Native::PeakEnergy]).values(kMonth) = tariff.gatherEnergy(kMonth)[(int)Period::Peak];
|
|
@jmarrec I agree that using either the accumulate or range based loop would eliminate this possible problem from popping back up. But also, this looks complete and ready to merge. I think let's just get this in to clean up develop for debug tests and continue on other PRs. Any objections? If not, this is going to the merge queue. |
|
OK, going in with this one. It will be great to get these working, but it will be notable to see the CI runtime impact. Expect slightly longer delays waiting on results. In the meantime I'll work on getting a supplementary machine to add to the corral. |
Pull request overview
So the RelWithDebugInfo build is awesome. Great runtime but still provides test coverage and catches some things. Unfortunately it does not by default catch array bounds or infinity checks. I added the flag that Julien had put in there to enable these checks. It revealed a couple dozen failures. Almost all of them were fixed with a tariff index off by one change. The only failing tests I see locally are:
integration.ShopWithPVandLiIonBattery
The PV/Battery file is almost certainly an infinity. The SSC uses infinity values as sentinels, so we need to handle this specially. I've looked into it in the past, but I don't recall hitting anything definitive. @jmarrec your thoughts would be highly appreciated.
integration.WCE_Diffuse_Shade
The WCE is hitting an array index. The slatAngIdxHi variable is -1, and the worker function that assigns these variables doesn't appear to be called for this setup. I started to dive in, but am also open to suggestion on why this might be happening. Here is the block where the array bound occurs. It is on the first Interp call, but happens before Interp is called. It happens while trying to set the
onstruction.effShadeBlindEmi[surfShade.blind.slatAngIdxHi]variable, and fails the lookupEnergyPlusFixture.EconomicTariff_LEEDtariff_with_Custom_Meter
This unit test is failing at one of the format protections that @jmarrec added. Here's a snippet of the backtrace from gdb, see line :
I'll chip away at these three and get this merged in. This will have a notable impact on test runtime, but it's worth it.
FYI @mjwitte @amirroth @jmarrec