simplify initial exchange #1305
Conversation
WalkthroughThis change adds Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
juliasloan25
force-pushed
the
js/simplify-init-exchange
branch
from
1360f58 to
050a82c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
experiments/ClimaEarth/components/atmosphere/climaatmos.jl(2 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(1 hunks)src/FluxCalculator.jl(3 hunks)test/flux_calculator_tests.jl(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- experiments/ClimaEarth/components/land/climaland_bucket.jl
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: test (1.11)
- GitHub Check: test (1.10)
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: docbuild
🔇 Additional comments (8)
src/FluxCalculator.jl (3)
130-141: Updated function signature for consistency with new surface humidity computation approach.The function now accepts an atmospheric simulation object instead of a thermodynamic state, matching the PR's goal of consistently computing surface humidity from atmospheric state.
143-156: Improved field type and density calculation implementation.Now retrieving element type from integrator and computing surface air density by fetching the thermodynamic state through the interface rather than using a passed parameter. This centralizes state retrieval and follows a more consistent pattern.
312-312: Updated call signature to pass atmospheric simulation instead of thermodynamic state.This update properly aligns with the function signature change made earlier.
test/flux_calculator_tests.jl (3)
77-91: Updated TestOcean surface_thermo_state signature for consistency.The function now correctly accepts an atmospheric simulation and retrieves the thermodynamic state from it, matching the changes in the main implementation.
111-122: Updated DummySurfaceSimulation3 implementation to use atmos_sim parameter.Keeps the implementation consistent with the interface changes.
210-210: Updated function call to use new signature.Fixed the call site to correctly pass the atmospheric simulation.
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (2)
218-218: Improved zero initialization to match target shape.Changed from a static vector to a zeros array matching the shape of the target field, which is more appropriate.
273-274: Enhanced specific_humidity calculation and added air_density getter.The specific humidity calculation now directly uses the moisture model's
ρq_totand divides by air density instead of using cached fields. The new air_density getter provides explicit access to this field, supporting the PR's goal of making surface humidity computation more consistent.
juliasloan25
force-pushed
the
js/simplify-init-exchange
branch
from
050a82c to
3b1df0e
Compare
juliasloan25
force-pushed
the
js/simplify-init-exchange
branch
from
3b1df0e to
abe42bf
Compare
juliasloan25
force-pushed
the
js/simplify-init-exchange
branch
2 times, most recently
from
ab0c060 to
3abcfe8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
NEWS.md (1)
11-12: Nitpick: wrap long lines. Lines 11–12 exceed the recommended 78-character limit. Consider breaking them into shorter lines for better readability in plain-text contexts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
NEWS.md(1 hunks)config/ci_configs/amip_target_topo_diagedmf_shortrun.yml(1 hunks)docs/src/interfacer.md(0 hunks)experiments/ClimaEarth/components/atmosphere/climaatmos.jl(2 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(1 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(0 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(0 hunks)experiments/ClimaEarth/setup_run.jl(2 hunks)experiments/ClimaEarth/user_io/debug_plots.jl(2 hunks)src/FieldExchanger.jl(2 hunks)src/FluxCalculator.jl(4 hunks)src/Interfacer.jl(0 hunks)src/surface_stub.jl(0 hunks)test/field_exchanger_tests.jl(0 hunks)test/flux_calculator_tests.jl(4 hunks)test/interfacer_tests.jl(0 hunks)
💤 Files with no reviewable changes (7)
- test/interfacer_tests.jl
- experiments/ClimaEarth/components/ocean/prescr_seaice.jl
- src/surface_stub.jl
- docs/src/interfacer.md
- test/field_exchanger_tests.jl
- experiments/ClimaEarth/components/ocean/slab_ocean.jl
- src/Interfacer.jl
🚧 Files skipped from review as they are similar to previous changes (8)
- config/ci_configs/amip_target_topo_diagedmf_shortrun.yml
- src/FluxCalculator.jl
- experiments/ClimaEarth/setup_run.jl
- experiments/ClimaEarth/user_io/debug_plots.jl
- src/FieldExchanger.jl
- experiments/ClimaEarth/components/land/climaland_bucket.jl
- experiments/ClimaEarth/components/atmosphere/climaatmos.jl
- test/flux_calculator_tests.jl
🧰 Additional context used
📓 Path-based instructions (1)
`*`: # CodeRabbit Style Guide (CliMA Inspired)
Leverage CodeRabbit for code reviews aligning with CliMA's practices.
I. Key Areas for CodeRabbit:
- Style: Naming (Titl...
*: # CodeRabbit Style Guide (CliMA Inspired)Leverage CodeRabbit for code reviews aligning with CliMA's practices.
I. Key Areas for CodeRabbit:
- Style: Naming (TitleCase, lowercase_with_underscores), line length (<78), indentation (4 spaces), import order.
- DRY: Flag duplicated code; encourage modularization.
- Docstrings: Flag missing docstrings for modules, structs, functions.
- Tests: Detect missing unit tests (if configured).
- Complexity: Report on cyclomatic complexity.
II. Conventions (CodeRabbit Can Help):
- Naming: Follow CliMA/CMIP conventions. Avoid
l,O,Ias single-char vars.- Unicode: Human review needed. Acceptable chars.
III. Documentation (CodeRabbit Flags Deficiencies):
- Docstrings: Describe purpose, inputs, outputs, examples.
IV. Testing (CodeRabbit: Missing Tests):
- Unit tests required for new/modified code.
V. CodeRabbit Config:
.coderabbit.yaml: Customize language, line length, tests.VI. Workflow:
- Review CodeRabbit's suggestions; investigate, provide context, address issues, improve config.
VII. Abandoned Code: Delete or clearly mark as deprecated.
VIII. Reviewer Responsibilities: Ensure adherence to best practices.
NEWS.md
🪛 markdownlint-cli2 (0.17.2)
NEWS.md
9-9: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.11 - macOS-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: ci 1.10 - macOS-latest
- GitHub Check: test (1.11)
- GitHub Check: docbuild
- GitHub Check: test (1.10)
- GitHub Check: ci 1.10 - ubuntu-latest
🔇 Additional comments (1)
NEWS.md (1)
9-15: Release note for PR#1305 is clear and correct. The new entry accurately summarizes the consistent computation of surface humidity from atmosphere state and surface temperature, and notes the removal of the initialstep!andreinit!calls.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
juliasloan25
force-pushed
the
js/simplify-init-exchange
branch
from
3abcfe8 to
35e583a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
NEWS.md (1)
9-15: Update documentation and ensure clarity
- The entry notes removal of
reinit!, but doesn’t mention that allInterfacer.get_field(_, Val(:surface_humidity))methods were removed—consider adding a brief line to capture this code change.- Static analysis (MD003) flagged an inconsistent heading style here. If the mix of setext and atx is intentional, you can disable the rule for this block:
<!-- markdownlint-disable MD003 --> #### Simplify initial component model exchange. PR[#1305](https://github.com/CliMA/ClimaCoupler.jl/pull/1305) <!-- markdownlint-enable MD003 -->🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
NEWS.md(1 hunks)config/ci_configs/amip_target_topo_diagedmf_shortrun.yml(1 hunks)docs/src/interfacer.md(0 hunks)experiments/ClimaEarth/components/atmosphere/climaatmos.jl(2 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(1 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(0 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(0 hunks)experiments/ClimaEarth/setup_run.jl(2 hunks)experiments/ClimaEarth/user_io/debug_plots.jl(2 hunks)src/FieldExchanger.jl(2 hunks)src/FluxCalculator.jl(4 hunks)src/Interfacer.jl(0 hunks)src/surface_stub.jl(0 hunks)test/field_exchanger_tests.jl(0 hunks)test/flux_calculator_tests.jl(4 hunks)test/interfacer_tests.jl(0 hunks)
💤 Files with no reviewable changes (7)
- test/interfacer_tests.jl
- experiments/ClimaEarth/components/ocean/prescr_seaice.jl
- src/Interfacer.jl
- experiments/ClimaEarth/components/ocean/slab_ocean.jl
- test/field_exchanger_tests.jl
- src/surface_stub.jl
- docs/src/interfacer.md
🚧 Files skipped from review as they are similar to previous changes (8)
- config/ci_configs/amip_target_topo_diagedmf_shortrun.yml
- src/FieldExchanger.jl
- experiments/ClimaEarth/user_io/debug_plots.jl
- test/flux_calculator_tests.jl
- src/FluxCalculator.jl
- experiments/ClimaEarth/setup_run.jl
- experiments/ClimaEarth/components/atmosphere/climaatmos.jl
- experiments/ClimaEarth/components/land/climaland_bucket.jl
🧰 Additional context used
📓 Path-based instructions (1)
`*`: # CodeRabbit Style Guide (CliMA Inspired)
Leverage CodeRabbit for code reviews aligning with CliMA's practices.
I. Key Areas for CodeRabbit:
- Style: Naming (Titl...
*: # CodeRabbit Style Guide (CliMA Inspired)Leverage CodeRabbit for code reviews aligning with CliMA's practices.
I. Key Areas for CodeRabbit:
- Style: Naming (TitleCase, lowercase_with_underscores), line length (<78), indentation (4 spaces), import order.
- DRY: Flag duplicated code; encourage modularization.
- Docstrings: Flag missing docstrings for modules, structs, functions.
- Tests: Detect missing unit tests (if configured).
- Complexity: Report on cyclomatic complexity.
II. Conventions (CodeRabbit Can Help):
- Naming: Follow CliMA/CMIP conventions. Avoid
l,O,Ias single-char vars.- Unicode: Human review needed. Acceptable chars.
III. Documentation (CodeRabbit Flags Deficiencies):
- Docstrings: Describe purpose, inputs, outputs, examples.
IV. Testing (CodeRabbit: Missing Tests):
- Unit tests required for new/modified code.
V. CodeRabbit Config:
.coderabbit.yaml: Customize language, line length, tests.VI. Workflow:
- Review CodeRabbit's suggestions; investigate, provide context, address issues, improve config.
VII. Abandoned Code: Delete or clearly mark as deprecated.
VIII. Reviewer Responsibilities: Ensure adherence to best practices.
NEWS.md
🪛 markdownlint-cli2 (0.17.2)
NEWS.md
9-9: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: test (1.11)
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: test (1.10)
|
I guess we can also remove |
|
This PR is behavior changing, but the longrun stability hasn't worsened, and the plots in regular CI look comparable to main, so I'll go ahead and merge |
Good point, I opened #1317 for that |
Purpose
This PR does 2 things which depend on each other:
q_sfcfrom atmosphere state and surface temperature, rather than sometimes computing and sometimes storing/retrieving in surface model cachestep!function to align with ittested longruns pipeline here
compare to previous longrun build here
Most of the longruns fail in both cases, but we see that
GPU AMIP FINE: new target amip: toporeaches OOM shortly after day 20 in both cases, andGPU AMIP FINE: new target amip: topo + diagedmfis stable past 34 weeks 4 days in both cases.closes #331
Content
q_sfcfrom the atmosphere thermo properties and surface temperatureqanymore; call radiation callback directly instead ofreinit!)step!to align with initial exchangeget_field(_, Val(:surface_humidity))functionsNext steps (to be done in #1293)
surface_thermo_statefunction