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

SOC can be spatially varying only #705

Closed
wants to merge 1 commit into from

Conversation

kmdeck
Copy link
Member

@kmdeck kmdeck commented Jul 24, 2024

Purpose

Our goal is to prescribed soil organic carbon from time varying data. We added this in, but our only use case currently was TimeVaryingInput((t) -> constant). Since SOC is a 3d field, I think the allocations in regridded_snapshot! led to a big slowdown with this change.
Before that PR:
https://buildkite.com/clima/climaland-benchmark/builds/709#0190a29f-b2ff-40e3-9dae-a007eae845c4 (10 seconds)
After.: https://buildkite.com/clima/climaland-benchmark/builds/744#0190b780-87fb-493d-ad30-3ee2aabf1d30 (25 seconds)

For now, I added the option to have SOC be only a spatially varying field and not use TimeVaryingInput (and hence not need regridded_snapshot!). this should speed things up for us, and also be relevant for shorter runs where SOC is not varying much, or for runs where we do not have time-varying data.

The time per simulation is now back to 10 seconds.

To-do

Content

Allow for the prescribed SOC driver to be a field, and not a TimeVaryingInput. In this case, set p.drivers.soc = the field each evaluate step, intead of evaluating the TimeVaryingInput object at a specific time.


  • I have read and checked the items on the review checklist.

@kmdeck kmdeck added the Run benchmarks Add this label to run benchmarks on clima label Jul 24, 2024
@Sbozzolo
Copy link
Member

Can you check this again once #704 is merged?

setup_prob is 86% of the execution, so maybe most of the time is spent in the setup only.

@kmdeck kmdeck force-pushed the kd/carbon_spatially_varying_only branch from 9525d2f to f4056e4 Compare July 25, 2024 15:08
@kmdeck
Copy link
Member Author

kmdeck commented Jul 25, 2024

@Sbozzolo this doesnt seem to affect the time to solution, though the flame graphs look different. I think I will close this for now, unless you want to take a look at the flame graphs for the soil canopy benchmark. weirdly, this PR seems to have more "update driver" allocations than the run associated with the PR you just merged.

@kmdeck kmdeck closed this Jul 26, 2024
@Sbozzolo
Copy link
Member

The time to solution should not be affected, only the flame graph should.

My PR essentially removed initialization from the flame graph so that we can focus on what happens at every timestep instead of only the first time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run benchmarks Add this label to run benchmarks on clima
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants