TDycores-Project / TDycore

BSD 2-Clause "Simplified" License
4 stars 0 forks source link

Diagnostic updates cause WY failures in branch with parallel fixes #224

Open jeff-cohere opened 2 years ago

jeff-cohere commented 2 years ago

Some recent updates have cause failures in the WY demos. @bishtgautam submitted PR #221, which applies several fixes to restore the dycore's parallel capabilities. Then PR #220 was merged, adding an interface for diagnostic fields. In a separate branch that contains the changes for #221 rebased to master post #220, the WY demos stopped working. Gautam's assessment:

The transient WY tests are failing. The residual for some cells is garbage at line 1167 of tdywy.c in the rebased branch. But, I can’t figure out where I made the mistake.

We're tracking that issue here so we can go ahead and merge #221.

We should probably add more parallel tests to our regression suite to make sure we don't damage the parallel runs in our future work.

jeff-cohere commented 2 years ago

Hey @bishtgautam , can you show me how to reproduce this issue? I'm wondering if it still occurs, and/or how much we should focus on fixing the WY tests when we know that discretization is going to be reworked.

nocollier commented 2 years ago

I thought we had discussed just removing it as needed? We can always reconstruct it later if it is needed.

On Thu, May 5, 2022 at 2:40 PM Jeffrey N. Johnson @.***> wrote:

Hey @bishtgautam https://github.com/bishtgautam , can you show me how to reproduce this issue? I'm wondering if it still occurs, and/or how much we should focus on fixing the WY tests when we know that discretization is going to be reworked.

— Reply to this email directly, view it on GitHub https://github.com/TDycores-Project/TDycore/issues/224#issuecomment-1118926253, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKFCB5BMUWTVVSCXUO75XLVIQI2XANCNFSM5NSGZFXA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

jeff-cohere commented 2 years ago

Yeah, maybe we should just "comment out" these regression tests for now. Thanks, @nocollier .