Branch never used in in `CalcASHRAETARPNatural ` — Vertical Surfaces or No delta T

[germolinal]

Issue overview

I was checking THIS LINE of code, in src/EnergyPlus/ConvectionCoefficients.cc, and I realised that we are comparing Floating point numbers through equality.

if ((DeltaTemp == 0.0) || (cosTilt == 0.0)) { // Vertical Surface
    // do something... but will the condition above ever be true?
}else{
   // other cases...
}

Considering the nature of IEEE Floating Point numbers, I suspect that this branch will virtually never be gone through. Am I wrong?

Proposed solution

How about we change this code to something like :

if ((abs(DeltaTemp) < 1e-5) || (abs(cosTilt) < 1e-5)) { // Vertical Surface
    // do something... Now it is more likely
}else{
   // other cases...
}

[Myoldmopar]

According to our test coverage, our own test suite causes that line to be hit 8909686 times.

[germolinal]

Interesting.... However, it was hit 8909686 times OUT OF 193404477 (4.6%, including unit tests?). I would expect it to be hit WAY more often. Wouldn't you? At least once for each vertical surface

[Myoldmopar]

Maybe? I'm not opposed to doing the comparison with a tolerance as you suggested, I really just wanted to check for myself and found it does get hit.

[germolinal]

All righty then, I'll just leave this here. Feel free to do as you wish!

[RKStrand]

@Myoldmopar So, do you think this is worth fixing? I would agree with its probably better to have some sort of range that establishes a surface being vertical.

[Myoldmopar]

A range does seem reasonable here, but I don't want to add calculations unnecessarily. This is getting hit many times during a simulation, and, as minimal as an abs might be, we still shouldn't just do it without thinking about other options. It would be really amazing if the curves just lined up so that we could just evaluate the curve and let them hit the correct regime automatically, so that we don't have to add extra conditional checks in here, but that's quite an ideal scenario.

I'm open to a pull request that adds a range and we can check whether there is any meaningful runtime impact or if a better option reveals itself through the review process. I would also suggest that the PR takes a look at the other code in this file to see if a similar solution could/should be applied elsewhere. But to save time, don't try to do that level of effort until review happens in case we need to modify the approach.

[mjwitte]

Are we really checking CostTilt==0.0 repeatedly? Seems we could tag surfaces that are vertical or horizontal once up front. (As long as the model isn't moving via Site:VariableLocation but I think that only moves the north axis, won't change tilts).

[Myoldmopar]

Fantastic point, and yes, even if the orientation is changing while the building is floating around the ocean, the tilt doesn't change. (Unless someone has data for real time wave patterns at like 5-10 m2 resolution and an algorithm to determine ship tilt that should be added to that code... :laughing: )

[Myoldmopar]

Probably worth a check everywhere in the code that we are checking CosTilt == or == CosTilt and similar variables related to tilt.

[amirroth]

This entire thread is on tilt.

[amirroth]

Also, @mitchute had previously implemented thisSurf.ConvOrientation during a local ConvectionCoefficient.cc refactor. Maybe that can be used here.

[jcyuan2020]

From an engineering point of view, how to define a "vertical" wall? 0 degree tilt, 0.001 degree, 1degree, or 5, 10, 15 degrees? Probably some engineer will say a 5-10 degree tilted wall should be well considered "vertical". Actually, there might be similar example on what kind of roof is pretty much considered a "horizontal" roof--it seems that engineers sometimes think with 15 degrees of tilting from "true horizontal"(90 degree) should be pretty much all well considered "horizontal".

[jcyuan2020]

Also, for double precision numbers, it seems that cosTilt == 0.0 has the accuracy about 1e-16 to 1e^-15 accuracy (or tolerance) at least.

[mitchute]

ConvectionCoefficients.cc is ripe for lots of refactoring 👀

[RKStrand]

Ok, with all that discussion, I'll bite and assign this to myself. I'll definitely take a look at the rest of ConvectionCoefficients.cc, search for more CosTilt comparisons, and follow @mjwitte suggestion about tagging surfaces. If anyone else has more suggestions, feel free to drop them in here. Sounds like there will be plenty of potential reviewers. 😂