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 grid be a positional argument to model constructors? #3250

Closed
glwagner opened this issue Sep 3, 2023 · 3 comments
Closed

Should grid be a positional argument to model constructors? #3250

glwagner opened this issue Sep 3, 2023 · 3 comments

Comments

@glwagner
Copy link
Member

glwagner commented Sep 3, 2023

The argument grid is required --- of course --- in model constructors.

Our current API makes grid a keyword argument. Originally (and it was a long time ago now) it was argued that keyword arguments are just better and that's why we should keep it. Also, we don't dispatch on it.

However, we violate that concept in Simulation, where the model is a positional argument.

I think it's more natural to input grid as a positional argument in the models. It's a relatively minor thing, but it avoids anti-patterns like grid=grid, which appeared in many scripts before ; grid was possible. It looks better.

This has come up because we are adding more Oceananigans-based AbstractModels over in ClimaSeaIce, and my first intuition there was to make grid a required positional argument. Ultimately though, we should strive for all models to have uniform interfaces, so either we change Oceananigans or we change ClimaSeaIce.

@navidcy
Copy link
Collaborator

navidcy commented Sep 7, 2023

I like kwarg because it’s more verbose.
Is there any other reason why it should be positional?
As you said grid=grid can be avoided although some people like it.

@glwagner
Copy link
Member Author

glwagner commented Sep 7, 2023

Hmm in that case, I can consider a grid keyword argument for other models too.

@glwagner
Copy link
Member Author

Is there any other reason why it should be positional?

I guess I'm just arguing that it's not consistent with most of the API, eg we write CenterField(grid).

I find the position representation of required arguments to be a natural and concise interface but perhaps I shouldn't die on that hill

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

No branches or pull requests

2 participants