Fix a minor bug in porous barriers #726
Conversation
Fix a bug that layer/interface weights may not be reset to 1.0 if no calculations are needed. The bug has negligible impact for barotropic applications but may affect baroclinic runs which are yet to performed.
herrwang0
force-pushed
the
fix_pb_bug
branch
from
12e7a0a
to
04785a5
Compare
|
Fails with different diagnostics in regression tests are to be expected, as by changing the default of |
There was a problem hiding this comment.
These changes seem sensible to me, and appear to be correctly implemented. I am tempted to approve them now.
However, it might make sense to hold these changes (especially the changes to the default value for USE_POROUS_BARRIER) until after the next PR from dev/gfdl to main. The PR immediately after that will include numerous other changes to defaults, as was agreed to in a late-July MOM6 dev call, and it might make sense to put this PR in along with the other PRs that will change defaults.
Hallberg-NOAA
added
bug
In this regard, does it make sense to break this PR into two? We could maybe fast-track a bug fix PR, so that it does not hinder some of the experiments we have in mind for regional runs? A second PR for default parameter change can be merged later. |
|
@herrwang0 It would be OK add the (Although maybe there is value in letting the regression fail, to signal a change in defaults? If so , then I think this needs discussion.) |
|
@marshallward Thanks for the response. In a way, I think it is informative to have these anticipated fails. But I guess this boils down to what is the standard practice for changing defaults. Can/should I change the default in MOM_input for tc tests? Or is it better to consolidate all default changes to a single (annual) PR? |
|
@herrwang0 The original purpose of the test is to catch unintentional regressions. I don't think any of us would consider a default parameter change to be a regression. So that would be the simplest answer to your question. But you make a good point. There is value in detecting and reporting changes in default parameters which change answers. But is this test the place to do it? There's a lot more to say here (GitHub's lack of a "Warn" state, the impact on later PRs to main, etc), but we should probably hold that discussion for another time. Personally I'd just change the tests, but I'm open to whatever you think is right. (However, I don't necessarily speak for the others!) |
|
I am breaking this PR into two, so that the bug fix can be adopted first. |
This PR fixes a minor bug in porous barriers module.
It may change answers for some very rare cases in barotropic runs. For an example I tested with barotropic tides, the answer is unchanged at bit level. Given the functionality is not yet widely used, I decide to omit a bug flag.
A second commit turns the default of
USE_POROUS_BARRIERto false, responding to a long standing request.