use nq=4 for boundary space #1369
Conversation
WalkthroughThis change updates the initialization of the Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
experiments/ClimaEarth/setup_run.jl (1)
233-233: Consider using a named constant instead of magic number.The hardcoded value
4achieves the PR objective but could benefit from better maintainability.+# Default quadrature points for independent boundary space (aligns with ClimaCore/ClimaAtmos defaults) +const DEFAULT_BOUNDARY_QUAD_POINTS = 4 + if share_surface_space boundary_space = CC.Spaces.horizontal_space(atmos_sim.domain.face_space) else h_elem = config_dict["h_elem"] - n_quad_points = 4 + n_quad_points = DEFAULT_BOUNDARY_QUAD_POINTS boundary_space = CC.CommonSpaces.CubedSphereSpace(FT; radius = FT(6371e3), n_quad_points, h_elem) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experiments/ClimaEarth/setup_run.jl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: ci 1.10 - macOS-latest
- GitHub Check: ci 1.11 - macOS-latest
- GitHub Check: test (1.10)
- GitHub Check: test (1.11)
- GitHub Check: docbuild
🔇 Additional comments (1)
experiments/ClimaEarth/setup_run.jl (1)
229-235: LGTM! Change successfully decouples boundary space from atmosphere.The implementation correctly addresses the PR objective by making boundary space creation independent of the atmosphere simulation. The value 4 aligns with stated defaults and maintains consistency.
Good point, it's out of date. I'll update it. |
Purpose
When we construct a boundary space not based on the atmosphere space, it should be entirely independent of the atmosphere simulation/space. It's helpful to be able to create the space without creating the atmosphere sim, especially when debugging.
This PR changes the independent boundary space to use
n_quad_points = 4. I chose this value because it's the default used by both ClimaCore and ClimaAtmos, and it's higher resolution thann_quad_points = 0used by the land. We can expose this as an option later on if users want to change it.closes #1365