Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we replace grid if halos are wrong, or throw an error? #2107

Open
glwagner opened this issue Dec 8, 2021 · 3 comments
Open

Should we replace grid if halos are wrong, or throw an error? #2107

glwagner opened this issue Dec 8, 2021 · 3 comments

Comments

@glwagner
Copy link
Member

glwagner commented Dec 8, 2021

Right now in the NonhydrostaticModel, we remake a user-provided grid if the halos are not big enough:

user_halo != required_halo && (grid = with_halo((Hx, Hy, Hz), grid)) # Don't replace grid unless needed.

This is a nice convenience for users experimenting with different advection schemes, closures, etc.

One major caveat, however, is that models that incorporate user-created fields, when those fields are provided at the time of model construction, may have been created on the "wrong" grid. This problem is most acute for PrescribedVelocityFields with the HydrostaticFreeSurfaceModel; however the issue is generic.

It doesn't appear to be very common for users to create their own fields, which is perhaps why no great problems have been detected. Nevertheless, this is an insidious "gotcha" that could lead to hard to define or undetectable bugs.

One solution is to throw an error if the grid's halos are not big enough, rather than recreating it. The error will instruct the user to specify larger halos. This damages usability (since users need to know and care about their halos) but makes user experiments more resilient against bugs.

In the future, if we also use large halos by default (eg (3, 3, 3) as proposed by #1245) then such an error would be encountered only rarely.

@simone-silvestri
Copy link
Collaborator

I am for removing the inflation and throwing an error. Might be annoying to always have to specify the halo in the grid (and the refractoring that will come with it) but it can save a lot of debugging for the users

@francispoulin
Copy link
Collaborator

I am also happy to throw and error and ask the use to specify a halo or at least the right size.

It also occurs to me that a user can specify a halo to be bigger than is required. I don't see any advantage of this and believe it would only slow things down. This is not worthy of an error but do we want to notify the user if they pick a halo of length 5 and they only need 1?

@simone-silvestri
Copy link
Collaborator

I agree, I am not too sure how much filling more than the necessary halos would hamper productivity but since that (especially on GPU) the halo filling is a non-negligible portion of the computation, it is best not to overdo it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants