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

Generalize atmos_init to dry and no radiation cases #754

Merged
merged 1 commit into from
May 4, 2024
Merged

Generalize atmos_init to dry and no radiation cases #754

merged 1 commit into from
May 4, 2024

Conversation

LenkaNovak
Copy link
Collaborator

@LenkaNovak LenkaNovak commented Apr 24, 2024 •
edited
Loading

closes #753

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

@LenkaNovak LenkaNovak force-pushed the ln/generalize-to-dry-norad branch from 2a3339e to f41929c Compare April 29, 2024 18:29
@LenkaNovak LenkaNovak mentioned this pull request Apr 29, 2024
Open
3 tasks
@LenkaNovak LenkaNovak marked this pull request as ready for review April 29, 2024 18:33
@LenkaNovak LenkaNovak requested a review from juliasloan25 April 29, 2024 22:40
juliasloan25
juliasloan25 reviewed Apr 29, 2024
View reviewed changes
Copy link
Member

@juliasloan25 juliasloan25 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! I especially like the directory setup change :) I noticed that we're using COUPLER_OUTPUT_DIR/COUPLER_REGRID_DIR in test/regridder_tests.jl too, would you mind changing that to use this new structure as well?

I was also concerned about some of the helper functions allocating new fields. If you want, we can brainstorm together how to avoid that

experiments/AMIP/user_io/io_helpers.jl Show resolved Hide resolved
experiments/AMIP/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved
experiments/AMIP/components/atmosphere/climaatmos.jl Show resolved Hide resolved
experiments/AMIP/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved
experiments/AMIP/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved
experiments/AMIP/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved
experiments/AMIP/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved
experiments/AMIP/components/atmosphere/climaatmos.jl Show resolved Hide resolved
@LenkaNovak LenkaNovak requested a review from juliasloan25 May 3, 2024 03:38
juliasloan25
juliasloan25 approved these changes May 3, 2024
View reviewed changes
Copy link
Member

@juliasloan25 juliasloan25 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! Just one note about removing the zero_scalar_... functions, but after that I think this is good to go

experiments/AMIP/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved
@LenkaNovak LenkaNovak force-pushed the ln/generalize-to-dry-norad branch 7 times, most recently from 069226f to 081d54a Compare May 4, 2024 16:01
@LenkaNovak
try
c76c080 
q_vap fix

syntax fix

clean

dry runs

dry runs

format

revs

try 0.0

broadcast zeros

CommonDataModel = "=0.3.5"

rebase

rebase

fix path

rebase

test rebase
@LenkaNovak LenkaNovak force-pushed the ln/generalize-to-dry-norad branch from 081d54a to c76c080 Compare May 4, 2024 16:58
@LenkaNovak LenkaNovak enabled auto-merge May 4, 2024 17:18
@LenkaNovak LenkaNovak merged commit ad4e9c4 into main May 4, 2024
7 checks passed
@juliasloan25 juliasloan25 deleted the ln/generalize-to-dry-norad branch May 7, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Generalize atmos_init to dry and no radiation cases
2 participants
@LenkaNovak @juliasloan25