(0.91.0) Make hydrostatic pressure anomaly optional in `NonhydrostaticModel`

[glwagner]

This PR makes the separation computation of the hydrostatic pressure anomaly an optional feature of NonhydrostaticModel. It's also off by default, which hopefully means that triply periodic simulations should work by default (@johnryantaylor a while ago you wanted to do a triply periodic simulation, hopefully this PR makes that possible).

@tomchor ready to review, let me know what you think about the design.

This PR supercedes #3080, where there is an extended description of this change plus a discussion about its pros and cons.

Closes #3364 (I think...)

[tomchor]

This is looking pretty good! My only comment is that it's not intuitive for a given user to figure out how to opt for a hydrostatic pressure separation. A flag called separate_hydrostatic_pressure that takes true/false would be much more intuitive, although we'd need a little more code. Should we prioritize user-friendliness here?

[glwagner]

This is looking pretty good! My only comment is that it's not intuitive for a given user to figure out how to opt for a hydrostatic pressure separation. A flag called separate_hydrostatic_pressure that takes true/false would be much more intuitive, although we'd need a little more code. Should we prioritize user-friendliness here?

This is more flexible, because sometimes users want to have access to the pressure field prior to model construction. If we use a flag, then we either can't support that or have to put some annoying logic in the constructor. Since I feel it'll be rare that people want to change this kwarg, I think the trade-offs work out that its better to have a simpler constructor even if those rare users that want to separate hydrostatic pressure have to build CenterField.

[tomchor]

This is looking pretty good! My only comment is that it's not intuitive for a given user to figure out how to opt for a hydrostatic pressure separation. A flag called separate_hydrostatic_pressure that takes true/false would be much more intuitive, although we'd need a little more code. Should we prioritize user-friendliness here?

This is more flexible, because sometimes users want to have access to the pressure field prior to model construction. If we use a flag, then we either can't support that or have to put some annoying logic in the constructor. Since I feel it'll be rare that people want to change this kwarg, I think the trade-offs work out that its better to have a simpler constructor even if those rare users that want to separate hydrostatic pressure have to build CenterField.

Got it! Agreed. Should we test that hydrostatic_pressure_anomaly is either CenterField(grid) or nothing and throw a helpful error otherwise?

[tomchor]

CC @zhazorken

[glwagner]

This is looking pretty good! My only comment is that it's not intuitive for a given user to figure out how to opt for a hydrostatic pressure separation. A flag called separate_hydrostatic_pressure that takes true/false would be much more intuitive, although we'd need a little more code. Should we prioritize user-friendliness here?

This is more flexible, because sometimes users want to have access to the pressure field prior to model construction. If we use a flag, then we either can't support that or have to put some annoying logic in the constructor. Since I feel it'll be rare that people want to change this kwarg, I think the trade-offs work out that its better to have a simpler constructor even if those rare users that want to separate hydrostatic pressure have to build CenterField.

Got it! Agreed. Should we test that hydrostatic_pressure_anomaly is either CenterField(grid) or nothing and throw a helpful error otherwise?

Sure.

[navidcy]

@glwagner the distributed tests are broken :(

[glwagner]

@glwagner the distributed tests are broken :(

easy to fix

[navidcy]

nice job!!