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

use new package versions #502

Merged
merged 1 commit into from
Nov 22, 2023
Merged

use new package versions #502

merged 1 commit into from
Nov 22, 2023

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Nov 10, 2023 •
edited by juliasloan25
Loading

closes #475 and #490

replaces #500, #498, #473

@juliasloan25 juliasloan25 changed the title Update coupler to use ClimaAtmos 0.16.3 cache use new package versions Nov 10, 2023
@juliasloan25 juliasloan25 force-pushed the gb/update_atmos_cache branch 2 times, most recently from b85f5bc to 8423092 Compare November 11, 2023 00:05
@LenkaNovak LenkaNovak mentioned this pull request Nov 11, 2023
Closed
13 tasks
@akshaysridhar akshaysridhar mentioned this pull request Nov 13, 2023
Closed
@akshaysridhar akshaysridhar self-requested a review November 14, 2023 00:32
akshaysridhar
akshaysridhar reviewed Nov 14, 2023
View reviewed changes
Copy link
Member

@akshaysridhar akshaysridhar 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.

Results from buildkite outcomes in the Slack thread LGTM (the run has been re-triggered, I'll review again once it is complete). Can we see if we can resolve the pending TODO statement with the updated package version?

experiments/ClimaCore/tc4_atm-lnd-sfc-fluxes/dummy_surface_fluxes.jl
@@ -94,7 +94,8 @@ function calculate_sfc_fluxes(formulation::DryMonin, parameters, θ_sfc, θ_1, u
z_0,
θ_basic,
z_in,
SF.FVScheme(),
SF.PointValueScheme(),
# TODO Check if statement below is still problematic
Copy link
Member

Choose a reason for hiding this comment

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

Before we merge this can we see if this #TODO block can be resolved ?

Copy link
Member

Choose a reason for hiding this comment

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

I expect this build https://buildkite.com/clima/climacoupler-ci/builds/2111 will verify if that iter count is still problematic.

Copy link
Member

Choose a reason for hiding this comment

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

we're planning on deleting this test case in the next couple of weeks, so I think it's okay to leave the TODO in this PR

experiments/AMIP/modular/components/land/bucket_init.jl Outdated Show resolved Hide resolved
LenkaNovak
LenkaNovak approved these changes Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@LenkaNovak LenkaNovak left a comment

Choose a reason for hiding this comment

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

LGTM after resolving @akshaysridhar 's comments (no major change in output diagnostics or performance, though please double check all runs once build_history is generated). Thank you all!

Project.toml Show resolved Hide resolved
Project.toml Show resolved Hide resolved
Project.toml Show resolved Hide resolved
@LenkaNovak LenkaNovak mentioned this pull request Nov 20, 2023
Closed
2 tasks
@juliasloan25 juliasloan25 force-pushed the gb/update_atmos_cache branch 5 times, most recently from 245a5ba to 6df4082 Compare November 22, 2023 17:25
@sriharshakandala
Copy link
Member

Can we remove UnPack package? Its functionality can be replaced by destructuring syntax supported in native Julia!

@juliasloan25
Copy link
Member

Can we remove UnPack package? Its functionality can be replaced by destructuring syntax supported in native Julia!

I want to get this PR merged today, so I'll remove UnPack separately. I opened an issue for it here #520

@juliasloan25
Copy link
Member

Plots do look slightly different, but I think it's within acceptable bounds given there are no overall trend changes and we've updated for a new SurfaceFluxes in this PR. I'll merge now

@juliasloan25 juliasloan25 merged commit 61af0de into main Nov 22, 2023
6 checks passed
This was referenced Nov 22, 2023
Closed
Closed
Closed
Closed
This was referenced Nov 27, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliasloan25 juliasloan25 juliasloan25 left review comments

@akshaysridhar akshaysridhar akshaysridhar left review comments

@LenkaNovak LenkaNovak LenkaNovak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update interfaces to match SF v0.8.0
5 participants
@Sbozzolo @sriharshakandala @juliasloan25 @LenkaNovak @akshaysridhar