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.

Sign up for GitHub

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

Jump to bottom

Support reproducible restarts with RRTMGP.jl #3382

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Support reproducible restarts with RRTMGP.jl #3382

merged 1 commit into from
Oct 18, 2024

Conversation

sriharshakandala
Copy link
Member

@sriharshakandala sriharshakandala commented Oct 14, 2024 •
edited
Loading

Purpose

Support reproducible restarts with RRTMGP.jl.
Seed random number generator with simulation time before calling update_fluxes.
The seeding can be turned on by setting radiation_reset_rng_seed to true. The default is set to false.

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

@sriharshakandala sriharshakandala force-pushed the sk/seed_radiation_solvers branch 3 times, most recently from 0304c5d to 9ea6f14 Compare October 14, 2024 19:00
@sriharshakandala sriharshakandala linked an issue Oct 14, 2024 that may be closed by this pull request
Seed the random number generator with the time-step number before calling the radiation solver to guarantee reproducibility. #3384
Closed
@sriharshakandala sriharshakandala force-pushed the sk/seed_radiation_solvers branch 5 times, most recently from ef91968 to bb30228 Compare October 17, 2024 20:59
@sriharshakandala sriharshakandala marked this pull request as ready for review October 17, 2024 21:01
@sriharshakandala
Copy link
Member Author

sriharshakandala commented Oct 17, 2024 •
edited
Loading

The end to end test calibration test seems to fail frequently, potentially due to problems with slurm daemon. This is unrelated to the current PR. @nefrathenrici is working on setting this test to soft fail

Sbozzolo
Sbozzolo reviewed Oct 17, 2024
View reviewed changes
Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, this change will make all the call to Random.seed! in all our drivers ineffective. As a result:

  • all the results should change
  • the nightly AMIP will break (because the nightly AMIP relies on setting the seed to run an ensemble) (@juliasloan25 @szy21 @trontrytel)

I suggest we make this option false by default and turn it on for the jobs we care about. In the nightly AMIP, we could manually add some small initial perturbation, or add a controllable offset to the seed.

Can you also update the NEWS file to describe this change and its implications?

Ideally, we also would like this to be tested, but the tests for restarts have no clouds. I don't know if we have any easy way to add this.

More broadly, given that this PR is not changing MSEs, I think this means that we are not testing any regime with clouds in our reproducibility tests.

test/dependencies.jl Show resolved Hide resolved
config/default_configs/default_config.yml Outdated Show resolved Hide resolved
src/parameterized_tendencies/radiation/RRTMGPInterface.jl Outdated Show resolved Hide resolved
src/solver/model_getters.jl Outdated Show resolved Hide resolved
@sriharshakandala
Copy link
Member Author

@nefrathenrici : I am setting the calibration_end_to_end_test to soft fail in the CI pipeline here!

szy21
szy21 reviewed Oct 17, 2024
View reviewed changes
Copy link
Member

@szy21 szy21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me after addressing Gabriele's comments, thanks!

@szy21
Copy link
Member

szy21 commented Oct 17, 2024

More broadly, given that this PR is not changing MSEs, I think this means that we are not testing any regime with clouds in our reproducibility tests.

Yes, that's probably true. We will change the initial condition to have some moisture so it will take shorter time to make clouds.

@sriharshakandala sriharshakandala force-pushed the sk/seed_radiation_solvers branch from ce57383 to d8648bd Compare October 18, 2024 17:00
@sriharshakandala
Copy link
Member Author

If there are no additional comments, I can go ahead and merge!

Sbozzolo
Sbozzolo reviewed Oct 18, 2024
View reviewed changes
Copy link
Member

@Sbozzolo Sbozzolo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I think two of my comments were not addressed, I left them again.)

Can you also please make the required change in ClimaCopuler?

src/parameterized_tendencies/radiation/RRTMGPInterface.jl Show resolved Hide resolved
src/solver/model_getters.jl Show resolved Hide resolved
NEWS.md Show resolved Hide resolved
@sriharshakandala sriharshakandala force-pushed the sk/seed_radiation_solvers branch 2 times, most recently from c29fbad to c630e36 Compare October 18, 2024 18:59
Sbozzolo
Sbozzolo approved these changes Oct 18, 2024
View reviewed changes
Copy link
Member

@Sbozzolo Sbozzolo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

(Please fix the bug with the string concatentation before merging)

Also, could you please take care of revelant changes in ClimaCoupler (to the nightly AMIP) as well?

@sriharshakandala sriharshakandala force-pushed the sk/seed_radiation_solvers branch from c630e36 to 279d131 Compare October 18, 2024 20:21
@sriharshakandala sriharshakandala added this pull request to the merge queue Oct 18, 2024
@sriharshakandala sriharshakandala removed this pull request from the merge queue due to a manual request Oct 18, 2024
@sriharshakandala
Support reproducible restarts with RRTMGP.jl.
0af8c7f 
Seed random number generator with simulation time before calling `update_fluxes`.
The seeding can be turned on by setting `radiation_reset_rng_seed` to `true`. The default is set to `false`.
@sriharshakandala sriharshakandala force-pushed the sk/seed_radiation_solvers branch from 01355e3 to 0af8c7f Compare October 18, 2024 21:41
@sriharshakandala sriharshakandala added this pull request to the merge queue Oct 18, 2024
Merged via the queue into main with commit 61f76b1 Oct 18, 2024
16 checks passed
@sriharshakandala sriharshakandala deleted the sk/seed_radiation_solvers branch October 18, 2024 23:21
@juliasloan25
Copy link
Member

Is this a breaking change (should I make today's release a minor release)?

@Sbozzolo
Copy link
Member

It's not breaking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nefrathenrici nefrathenrici nefrathenrici left review comments

@szy21 szy21 szy21 left review comments

@trontrytel trontrytel trontrytel left review comments

@Sbozzolo Sbozzolo Sbozzolo approved these changes

@charleskawczynski charleskawczynski charleskawczynski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@sriharshakandala @szy21 @juliasloan25 @Sbozzolo @trontrytel @charleskawczynski @nefrathenrici