-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Floating point exceptions by default in Debug mode (GCC/Clang) #10578
Conversation
@Myoldmopar Shouldn't these errors be investigated and fixed (either the code needs fixing or the test initialization itself)? |
Yes, are you asking me to do this investigation and fix things? If so, I can add it to my list and get to it soon. If not, are you wanting to? And as an aside, we are in the unfortunate position of including a third party library that uses NaN intentionally as a signal value. I'm not sure what will come of this, but I wanted to offer a small heads up here. |
Fixes EnergyPlusFixture.DivisorNormalizationDivisorOnlyButItIsZero
The issue was first unitialized StdRhoAir and co, but the main problem was getting the test to pass. Took me a while to figure out...
cf #10470 (review) @RKStrand does that mean sense to you please?
…oils_TestComponentSizingOutput_TwoSpeed
…ergyPlusFixture.TwoSpeedDXCoilStandardRatingsTest
91879ba
to
023b7f8
Compare
After @jmarrec enabled floating point exception catching on Mac, and found a whole bunch of failing tests, he got to work fixing them up. There are unfortunately 5 that are still remaining, and because of their nature (sizing, coils, ...), it would be great to get insight from @rraustad . I'm inclined to just disable the FPE on this branch long enough to merge it in, then open a new branch where they fail again, and resolve them there. Thoughts anyone? |
So what is the method to find these FP exceptions? Run unit tests in a debug build? That's very painful. |
I can't believe how long this took me, but I think I got it working for windows too, in 64018cf Turns out the gtest was not enabling floating point exceptions for msvc (ever). I added a I'm testing it on MSVC now, will consider adding it to this branch if it behaves properly. |
Alright, confirmed it appears to work fine on MSVC, so I added it to this branch.
In release, after calling
|
@jmarrec this is great, thanks for another major effort. @rraustad could you confirm that these tests fail locally for you now? I'd love to know whether to just wait here until they are finally fixed and merge this all at once, or get this merged in as nearly-done and finish on a separate branch. Thanks all! |
@Myoldmopar working as expected. I could push the fix for DXCoils_Test2 which also fixes TestMultiSpeedCoilsAutoSizingOutput. The MultispeedDX* failures may take a little time to figure out the Nan. |
@rraustad feel free to push indeed! thanks |
Down to 9! Looks like they are 3 polygon and 6 ITE, hopefully just two more one-liners! :) |
I have no idea why fixing the crash causes the SurfSunlitFrac values to change. 1) the non-zero values are for the mirrored shading surface now, and 2) the south was has a 0 sunlit fraction ?? |
src/EnergyPlus/SolarShading.cc
Outdated
@@ -3947,11 +3947,11 @@ void CLIPLINE(Real64 &x1, Real64 &x2, Real64 &y1, Real64 &y2, Real64 maxX, Real6 | |||
else if (c2 == 3) { | |||
visible = true; | |||
x1 = minX; | |||
y1 = maxY + e / dx; | |||
y1 = maxY + General::SafeDivide(e, dx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be a better way to correct this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -2212,7 +2212,7 @@ TEST_F(EnergyPlusFixture, AirflowNetwork_TestPressureStat) | |||
|
|||
// Check indoor pressure and mass flow rate | |||
EXPECT_NEAR(PressureSet, state->afn->AirflowNetworkNodeSimu(3).PZ, 0.0001); | |||
EXPECT_NEAR(0.00255337, state->afn->ReliefMassFlowRate, 0.0001); | |||
EXPECT_NEAR(0.06551, state->afn->ReliefMassFlowRate, 0.0001); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume since StdRhoAir is now in the fixture that something has changed BEFORE this unit test sets StdRhoAir.
Looks like I forgot to add the FPE fix for PolygonClippingDirect
and looking at the reason for the CI failures again for SolarShadingTest_PolygonOverlap2 and 3. The south wall has coordinates:
and the south shade nearest the wall has coordinates:
with the outer shade having coordinates:
So the outer shade is the same size as the inner shade which are larger than the wall. OK fine. These surfaces face North and location is Chicago, so the sun is behind these surfaces? And then seeing the SurfSunlitFrac = 1 for the mirrored shading surfaces would make sense? I still can't figure out why the wall and window SurfSunlitFrac = 0 when the shade transmittance = 1. |
@Myoldmopar I have done as much as I can here. |
@rraustad thank you very much! |
The SUNCOSTS being (0.2, 0.2, 0.2) is unrealistic. Gotten from running 1ZoneUncontrolled.idf with chicago weather on Jan 1 at 12:
THe South Wall has a OutNormVec being OutNormVec = (x = 0, y = -1, z = 0), so when using 0.2 for the SUNCOSTS y, you end up with a negative number. |
Gotten from running 1ZoneUncontrolled.idf with chicago weather on Jan 1 at 12
Thanks @jmarrec that did the trick. The SurfSunlitFrac values are back to the original values. However, I am confused as to how these unit tests passed when the SUNCOS data was not provided. |
@rraustad We shouldn't be too concerned about this (we shouldn't play wiht NaN to begin with), but to satisfy the curiosity: In EnergyPlus/src/EnergyPlus/SolarShading.cc Lines 5022 to 5032 in 52fee4b
IEEE 754 says that operator < returns false
https://en.wikipedia.org/wiki/NaN A functionally equivalent test code is this: https://godbolt.org/z/sq5nxj6Gz state.dataSolarShading->SurfSunCosTheta is NaN as well. cf EnergyPlus/third_party/ObjexxFCL/src/ObjexxFCL/TypeTraits.hh Lines 633 to 666 in 52fee4b
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I look forward to using this in the future.
Pull request overview
Realize there was still a TODO... Let's see if CI complains or not now.
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.