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

Remove unused fieldvector #2131

Merged
merged 2 commits into from
Sep 21, 2023
Merged

Remove unused fieldvector #2131

merged 2 commits into from
Sep 21, 2023

Conversation

charleskawczynski
Copy link
Member

Followup to #2130

@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Sep 20, 2023
2131: Remove unused fieldvector r=charleskawczynski a=charleskawczynski

Followup to #2130

Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
@bors
Copy link
Contributor

bors bot commented Sep 20, 2023

Build failed:

@charleskawczynski
Copy link
Member Author

Why is

        Fields.bycolumn(axes(Y.c)) do colidx
            precipitation_tendency!(p.Yₜ, Y, p, t, colidx, p.precip_model)
        end # TODO: Set the diagnostics without computing the tendency.

used in the diagnostics? cc @trontrytel . Can we split this into precompute and add a function that adds fluxes so that we can remove p.Yₜ?

@trontrytel
Copy link
Member

Not sure why? Not wanting to cache more things and or duplicate the code?

I don't have the bandwidth to fix this this week. Feel free to open an issue and tag me on it. I can try getting it done before the next sprint

used in the diagnostics? cc @trontrytel . Can we split this into precompute and add a function that adds fluxes so that we can remove p.Yₜ?

@szy21
Copy link
Member

szy21 commented Sep 21, 2023

I think we might use it to make sure the precipitation tendency is consistent with the state, as we call set_precomputed_quantities! in the diagnostics which updates the thermo state. It might be ok to remove it (i.e. I think it's ok if the diagnostics we save are slightly inconsistent, but maybe other people have different opinions.)

@charleskawczynski
Copy link
Member Author

I think we might use it to make sure the precipitation tendency is consistent with the state, as we call set_precomputed_quantities! in the diagnostics which updates the thermo state. It might be ok to remove it (i.e. I think it's ok if the diagnostics we save are slightly inconsistent, but maybe other people have different opinions.)

I think it's fine to precompute things needed for precipitation tendencies, but the precip tendendencies themselves don't seem to be used in the diagnostics-- Also if they are, then I think we can get them from the integrator?

@charleskawczynski
Copy link
Member Author

@trontrytel, for now, I've just wrapped the tendency updates in if-blocks (if !isnothing(Yₜ); Yₜ.var += ...; end), so that we can pass in nothing from diagnostics-- all other quantities are computed (so p should be appropriately updated). Is this okay for now?

@szy21
Copy link
Member

szy21 commented Sep 21, 2023

I think we might use it to make sure the precipitation tendency is consistent with the state, as we call set_precomputed_quantities! in the diagnostics which updates the thermo state. It might be ok to remove it (i.e. I think it's ok if the diagnostics we save are slightly inconsistent, but maybe other people have different opinions.)

I think it's fine to precompute things needed for precipitation tendencies, but the precip tendendencies themselves don't seem to be used in the diagnostics-- Also if they are, then I think we can get them from the integrator?

We calculate col_integrated_rain and col_integrated_snow in precipitation_tendency!, that's probably why we call it here. We can move them somewhere else though - we don't need them in the tendency.

@trontrytel
Copy link
Member

@trontrytel, for now, I've just wrapped the tendency updates in if-blocks (if !isnothing(Yₜ); Yₜ.var += ...; end), so that we can pass in nothing from diagnostics-- all other quantities are computed (so p should be appropriately updated). Is this okay for now?

Sounds good. Apologies it's such a mess over there. It would be great to allocate some time and straighten things up (looking at myself pointedly).

The logic we want would be to put compute_precipitation_cache somewhere in precomputed_quantities, instead of calling it from the tendency? And then use it from both the tendency and diagnostics?

@charleskawczynski
Copy link
Member Author

Sounds good. Apologies it's such a mess over there. It would be great to allocate some time and straighten things up (looking at myself pointedly).

The logic we want would be to put compute_precipitation_cache somewhere in precomputed_quantities, instead of calling it from the tendency? And then use it from both the tendency and diagnostics?

No problem. I'm just trying to incrementally improve on CliMA/ClimaCore.jl#1467 for now. It's not clear to me how to best structure things, but there's definitely room for improvement.

@charleskawczynski
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 21, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit a515e64 into main Sep 21, 2023
7 of 8 checks passed
@bors bors bot deleted the ck/cleanup branch September 21, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants