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

Enforce consistent view of compute graph #1145

Open
charleskawczynski opened this issue Jun 8, 2022 · 0 comments
Open

Enforce consistent view of compute graph #1145

charleskawczynski opened this issue Jun 8, 2022 · 0 comments

Comments

@charleskawczynski
Copy link
Member

Using an auxiliary field has some advantages-- we can precompute / cache potentially expensive fields, and then reuse them in multiple places.

One major disadvantage of not computing everything on-the-fly, though, is that we can have an inconsistent view of the compute graph (i.e., compute graph leading to tendencies). For example:

x = center_aux_grid_mean(state)
y = center_aux_grid_mean(state)
z = center_aux_grid_mean(state)
@. y = x
@. x = 1
@. z = y

This may seem obvious, but it's not trivial because of how complex our tendency calculations are.

Several ideas were proposed.

@trontrytel suggested we add a Bool per flag, and then we can check that setindex! is called before getindex for every variable. This would work, but it would require that we add meta information to Fields (and propagate that when we use, e.g., copy), which seems like it might be a bit of work. Also, the debugging mode would likely not be performant.

@simonbyrne suggested we fill the aux field with NaNs at the begging of update_aux! (I think @trontrytel suggested this for ICs a while back, too), which I quite like. It's very simple, not very invasive, and wouldn't be a big performance overhead.

The only caveat with both of these ideas is that some fields don't need to be updated, and so filling with NaNs would kill the unchanged ICs. #1128 suggests that we could enforce that more variables are computed on-the-fly, however, this isn't practical for the reference state.

One way that we could make this work more consistently is if we declared, ahead of time, which variables are fixed in time, and which are dynamically updating. This was actually discussed several times in ClimateMachine, and I think it's worth a revisit.

I'll open an issue about adding a fixed aux state-- this problem is very elusive, and we'll continue running into it until we have safe guards in place.

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

1 participant