Fix erroneous parsing of time table.

marchdf commented 4 months ago

Summary

This was a weird one and threw me for a loop. Basically I was getting non-deterministic FPEs (nan) when running the regression tests with MPI. Wouldn't show up in debug, single rank builds (probably wasn't trying enough). Turns out we were trying to parse the header of the time table (strings) into reals. This meant that m_vel_speed would sometimes be a valid real and sometimes it would be a nan. Not sure what the deal is there. The fix is to skip the header and read the data in properly. We do this in other file reads so this should have been done here as well. But the problem wouldn't show up consistently so it wasnt caught.

Pull request type

Please check the type of change introduced:

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, renaming)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] Documentation content changes
[ ] Other (please describe):

Checklist

This PR was tested by running:

the unit tests
- [x] on GPU
- [x] on CPU
the regression tests
- [x] on GPU
- [x] on CPU

gpu_tests.txt cpu_tests.txt

moprak-nrel commented 4 months ago

Great catch, looking at the full table parser this makes sense -- Looks like this may have been introduced in #655 https://github.com/Exawind/amr-wind/blob/ea448365033fc6bc9ee0febeb369b377f4fd8240/amr-wind/equation_systems/icns/source_terms/ABLForcing.cpp#L25-L36

mbkuhn commented 4 months ago

Good catch. I saw that it had skipped the header line in ABLForcing, which is why I added a header to the timetable file in the reg test. Prior to adding that header, I did confirm that ABLForcing was skipping the first line of valid data. However, I didn't check that it was consistent with ABLFieldInit.

Bummer that this didn't get in prior to v2.0.0

Exawind / amr-wind