Use default_num_points when available #3774
Conversation
WalkthroughThe update removes inline logic for estimating the number of NetCDF points in Suggested labels
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (12)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/callbacks/get_callbacks.jl(1 hunks)src/compat.jl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- 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: downstream ClimaCoupler.jl (1.11)
- GitHub Check: downstream ClimaCoupler.jl (1.10)
- GitHub Check: ci 1.10 - macOS-latest
- GitHub Check: test (1.11)
- GitHub Check: evaluate
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: test (1.10)
- GitHub Check: docbuild
- GitHub Check: format
Sbozzolo
force-pushed
the
gb/default_num_points
branch
3 times, most recently
from
59ba581 to
15f03a1
Compare
|
@Sbozzolo Is there a reason this hasn't been reviewed/merged? |
I ran into this problem with the macOS test and I couldn't debug it. I'll rebase and see if now this works |
Sbozzolo
force-pushed
the
gb/default_num_points
branch
2 times, most recently
from
2cbe413 to
304c4f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/compat.jl (1)
121-122: Improve error message specificityThe current error message doesn't indicate which config value caused the error.
- error("Uncaught case") + error("Uncaught config case: $(parsed_args["config"])")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.buildkite/Manifest-v1.11.toml(5 hunks)src/callbacks/get_callbacks.jl(1 hunks)src/compat.jl(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .buildkite/Manifest-v1.11.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/callbacks/get_callbacks.jl
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: test (1.10)
- GitHub Check: test (1.11)
- GitHub Check: evaluate
- GitHub Check: docbuild
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: downstream ClimaCoupler.jl (1.11)
- GitHub Check: downstream ClimaCoupler.jl (1.10)
- GitHub Check: ci 1.10 - ubuntu-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
🔇 Additional comments (2)
src/compat.jl (2)
4-4: Good import addition for ClimaDiagnosticsThis import is necessary for the new version-dependent functionality.
126-128: Good use of newer ClimaDiagnostics APIThe function correctly delegates to the new ClimaDiagnostics function when available, which centralizes the calculation logic.
| if pkgversion(ClimaDiagnostics) < v"0.2.14" | ||
| function default_netcdf_points(space, parsed_args) | ||
| # Estimate the number of points we need to cover the entire domain | ||
| # ncolumns is the number of local columns | ||
| tot_num_columns = | ||
| ClimaComms.nprocs(ClimaComms.context(space)) * | ||
| Fields.ncolumns(space) | ||
| if parsed_args["config"] == "plane" | ||
| num1, num2 = tot_num_columns, 0 | ||
| elseif parsed_args["config"] == "sphere" | ||
| num2 = round(Int, sqrt(tot_num_columns / 2)) | ||
| num1 = 2num2 | ||
| elseif parsed_args["config"] == "box" | ||
| num2 = round(Int, sqrt(tot_num_columns)) | ||
| num1 = num2 | ||
| elseif parsed_args["config"] == "column" | ||
| # We need at least two points horizontally because our column is | ||
| # actually a box | ||
| num1, num2 = 2, 2 | ||
| else | ||
| error("Uncaught case") | ||
| end | ||
| return (num1, num2, Spaces.nlevels(space)) | ||
| end | ||
| else | ||
| function default_netcdf_points(space, _) | ||
| return ClimaDiagnostics.Writers.default_num_points(space) | ||
| end | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing imports for Fields and ClimaComms
The function uses ClimaComms and Fields modules but there are no corresponding import statements visible in this file.
🏁 Script executed:
#!/bin/bash
# Check if Fields and ClimaComms are imported elsewhere
grep -r "import.*Fields" --include="*.jl" .
grep -r "import.*ClimaComms" --include="*.jl" .Length of output: 5776
Let’s inspect the top of src/compat.jl for any import/using statements:
🏁 Script executed:
#!/bin/bash
grep -n '^import' src/compat.jl
grep -n '^using' src/compat.jlLength of output: 199
Add missing imports for Fields and ClimaComms
The default_netcdf_points function calls Fields.ncolumns and ClimaComms.nprocs/ClimaComms.context, but neither module is imported in src/compat.jl.
• File: src/compat.jl – at the top, alongside the other import statements
• Add:
import ClimaCore
+import ClimaCore: Fields
+import ClimaComms
import ClimaUtilities
import ClimaCore: Domains, Spaces, Topologies
import ClimaDiagnostics📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if pkgversion(ClimaDiagnostics) < v"0.2.14" | |
| function default_netcdf_points(space, parsed_args) | |
| # Estimate the number of points we need to cover the entire domain | |
| # ncolumns is the number of local columns | |
| tot_num_columns = | |
| ClimaComms.nprocs(ClimaComms.context(space)) * | |
| Fields.ncolumns(space) | |
| if parsed_args["config"] == "plane" | |
| num1, num2 = tot_num_columns, 0 | |
| elseif parsed_args["config"] == "sphere" | |
| num2 = round(Int, sqrt(tot_num_columns / 2)) | |
| num1 = 2num2 | |
| elseif parsed_args["config"] == "box" | |
| num2 = round(Int, sqrt(tot_num_columns)) | |
| num1 = num2 | |
| elseif parsed_args["config"] == "column" | |
| # We need at least two points horizontally because our column is | |
| # actually a box | |
| num1, num2 = 2, 2 | |
| else | |
| error("Uncaught case") | |
| end | |
| return (num1, num2, Spaces.nlevels(space)) | |
| end | |
| else | |
| function default_netcdf_points(space, _) | |
| return ClimaDiagnostics.Writers.default_num_points(space) | |
| end | |
| end | |
| # At the top of src/compat.jl, alongside the other imports: | |
| import ClimaCore | |
| +import ClimaCore: Fields | |
| +import ClimaComms | |
| import ClimaUtilities | |
| import ClimaCore: Domains, Spaces, Topologies | |
| import ClimaDiagnostics |
🤖 Prompt for AI Agents
In src/compat.jl around lines 101 to 129, the function default_netcdf_points
uses the Fields and ClimaComms modules but these are not imported in the file.
To fix this, add import statements for Fields and ClimaComms at the top of
src/compat.jl alongside the existing imports. This will ensure the function can
access Fields.ncolumns and ClimaComms.nprocs/context without errors.
`ClimaDiagnostics` introduced a function to compute the default number of points for the NetCDFWriter. This commit uses this new function when available.
Sbozzolo
force-pushed
the
gb/default_num_points
branch
from
304c4f3 to
54862bd
Compare
|
This uncovered an edge case in ClimaAnalysis. It got fixed in CliMA/ClimaAnalysis.jl@b014f0a |
ClimaDiagnosticsintroduced a function to compute the default number of points for the NetCDFWriter. This commit uses this new function when available.