add OceananigansSimulation template #1253
Conversation
WalkthroughThis change introduces a new file for ocean simulations within the ClimaCoupler framework. It defines the Changes
Possibly related issues
Suggested reviewers
🪧 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/ocean_template
branch
from
3fb0ad4 to
2a81f44
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (2)
experiments/ClimaEarth/components/ocean/oceananigans.jl (2)
15-19: Consider documenting struct type parameters more explicitly.
Though the docstring lists the fields inOceananigansSimulation, it might also help to clarify howM,I, andAare intended to be used (e.g., constraints onMfor the ocean model, etc.). This can make future refactors and debugging easier.
250-252: Implement or stub outget_model_prog_statein a clearer way.
It raises an error, which is acceptable if you’re not ready to support checkpointing. However, you might provide a minimal implementation returningnothingor an emptyFieldVectorto safely integrate with the checkpointing system in the near future.
🛑 Comments failed to post (4)
experiments/ClimaEarth/components/ocean/oceananigans.jl (4)
86-124: 🛠️ Refactor suggestion
Implement or remove unneeded
update_field!stubs.
All these stub methods have “TODO fill this out” placeholders. Clarify which fields are genuinely needed by your ocean model. Unused ones can be removed to reduce confusion. If they are needed, ensure the method properly remapsfieldontosim.modelso that future flux computations rely on correct grid values.Do you want help generating partial implementations for any of these stubs?
213-239: 🛠️ Refactor suggestion
Add logic for computing surface fluxes or remove placeholders.
Currently, the callOceananigans.compute_surface_fluxes(p, sim.model, Y, t, atmos_sim.integrator)references undefined variablesp,Y,t. Either define or pass them. If this is a placeholder, consider raising aNotImplementedErroror removing it until ready. This helps avoid runtime confusion.
131-144:
⚠️ Potential issueUndefined variable
air_density
InsideFieldExchanger.update_sim!, the function callsInterfacer.update_field!(sim, Val(:air_density), air_density), butair_densityis not defined in this scope. It seems you intended to usecsf.air_density, mirroring the pattern used for precipitation and radiative fields.Apply this diff to fix the variable reference:
- Interfacer.update_field!(sim, Val(:air_density), air_density) + Interfacer.update_field!(sim, Val(:air_density), csf.air_density)📝 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.function FieldExchanger.update_sim!(sim::OceananigansSimulation, csf, turbulent_fluxes, area_fraction) Interfacer.update_field!(sim, Val(:air_density), csf.air_density) Interfacer.update_field!(sim, Val(:area_fraction), area_fraction) # precipitation Interfacer.update_field!(sim, Val(:liquid_precipitation), csf.P_liq) Interfacer.update_field!(sim, Val(:snow_precipitation), csf.P_snow) # update fields for radiative transfer Interfacer.update_field!(sim, Val(:sw_d), csf.SW_d) Interfacer.update_field!(sim, Val(:lw_d), csf.LW_d) # TODO update other fields as needed end
32-43: 🛠️ Refactor suggestion
Instantiate
modelandintegratorbefore returning.
Right now, you returnOceananigansSimulation(model, integrator, area_fraction)but neithermodelnorintegratoris defined in the constructor. Consider instantiating them (e.g.,Oceananigans.Model(...)) and your integrator. For example:function OceananigansSimulation( ::Type{FT}, dt::TT, tspan::Tuple{TT, TT}, start_date::Dates.DateTime, output_dir::String, area_fraction, ) where {FT, TT <: Union{Float64, ITime}} # TODO fill this out - return OceananigansSimulation(model, integrator, area_fraction) + model = Oceananigans.Model(...) # For example, specify the domain, physics, etc. + integrator = some_integrator_creation(..., dt) + return OceananigansSimulation(model, integrator, area_fraction)📝 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.function OceananigansSimulation( ::Type{FT}, dt::TT, tspan::Tuple{TT, TT}, start_date::Dates.DateTime, output_dir::String, area_fraction, ) where {FT, TT <: Union{Float64, ITime}} # TODO fill this out model = Oceananigans.Model(...) # For example, specify the domain, physics, etc. integrator = some_integrator_creation(..., dt) return OceananigansSimulation(model, integrator, area_fraction) end
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
experiments/ClimaEarth/components/ocean/oceananigans.jl (2)
62-67: Unimplemented field getters.These
get_fieldmethods currently returnnothing. If these fields are essential for coupled flux calculations (e.g., computing roughness length), leaving them unimplemented could block development.Would you like help creating placeholder fields or implementing these lookups? I can open an issue or provide sample code.
250-251: Unimplemented checkpointer function.
error("get_model_prog_state not implemented")blocks usage of the restart system. This is expected to return aClimaCore.FieldVectoror similarly typed state for checkpointing.You might implement a minimal version returning state fields from
sim.model. Otherwise, checkpointing and restart features will remain unavailable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experiments/ClimaEarth/components/ocean/oceananigans.jl(1 hunks)
🔇 Additional comments (2)
experiments/ClimaEarth/components/ocean/oceananigans.jl (2)
52-53: Potential off-by-one in integration time.
Interfacer.step!(sim.integrator, t - sim.integrator.t, true)may skip or repeat steps iftis not exactly one step beyondsim.integrator.t. Verify the correctness of subtractingsim.integrator.tfromt.Would you like to run a check to confirm the time-stepping behavior aligns with your desired timeline? If so, I can generate a script to inspect and compare the integrator’s
tvalues after each step.
213-239: Incomplete flux calculation referencing undefined variables.Lines 223-224 call
Oceananigans.compute_surface_fluxes(p, sim.model, Y, t, atmos_sim.integrator), butp,Y, andtare undeclared here. Until you implement the real flux logic (or supply these variables properly), calling this will fail.If you need to gather evidence of how these variables are declared in other files, I can generate a script to search the repository for their definitions or usage to confirm the correct references.
juliasloan25
force-pushed
the
js/ocean_template
branch
from
2a81f44 to
6134564
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
experiments/ClimaEarth/components/ocean/oceananigans.jl (4)
61-77: Severalget_fieldmethods returnnothing.These placeholders hinder proper data exchange. If you need these fields, implement the logic; if they’re unnecessary, remove them to avoid confusion.
Would you like me to help fill in or remove these placeholders?
90-92: Empty implementation forVal(:air_density)update.This usage is unimplemented. If air density is integral to ocean model calculations, please add logic to remap and update its values.
Shall I propose a code snippet to properly integrate and remap the input field?
223-223: References top,Y, andtare placeholders and undefined.Use actual variables from the simulation state or remove this call until you're ready to compute surface fluxes.
Would you like me to open an issue or propose a scaffold for initializing these variables?
250-252:get_model_prog_stateis unimplemented.If you intend to leverage the restart system or checkpointing, implement a ClimaCore-like
FieldVectorretrieval. Otherwise, consider removing it until it’s needed.Do you want help drafting an implementation that wraps your model state into a
FieldVector?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experiments/ClimaEarth/components/ocean/oceananigans.jl(1 hunks)
🔇 Additional comments (2)
experiments/ClimaEarth/components/ocean/oceananigans.jl (2)
32-43: Constructor still references undefinedmodelandintegrator.This mirrors a previous comment and remains unresolved. Both variables must be defined or passed in before constructing the
OceananigansSimulation.
131-132: Undefined variableair_density.This is the same issue previously flagged and remains unresolved. Replace
air_densitywith something likecsf.air_densityor pass it as a function argument:- Interfacer.update_field!(sim, Val(:air_density), air_density) + Interfacer.update_field!(sim, Val(:air_density), csf.air_density)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
experiments/ClimaEarth/Project.toml (2)
12-12: Add version constraints for ClimaOcean
Currently, there's no[compat]entry forClimaOcean. Consider specifying a version range in the[compat]section to ensure that updates toClimaOceandon’t inadvertently break your code.
21-21: Pin or add version constraints for Oceananigans
Likewise, consider adding a[compat]entry forOceananigansso that your environment remains stable and reproducible across potential major version changes.experiments/ClimaEarth/components/ocean/oceananigans.jl (2)
17-20: Clarify the naming ofsimfield
Renaming thesimfield (e.g., tomodelorsimulation) could reduce confusion, sincesimtypically refers to the whole simulation object while this field specifically appears to hold the Oceananigans simulation struct.
101-163: Implement or remove placeholder methods
ManyInterfacer.get_fieldandInterfacer.update_field!methods currently returnnothingor containTODOcomments. Please fill in the required logic (such as remapping to the ocean grid) or remove them if not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
experiments/ClimaEarth/Manifest-v1.11.toml(42 hunks)experiments/ClimaEarth/Manifest.toml(42 hunks)experiments/ClimaEarth/Project.toml(2 hunks)experiments/ClimaEarth/components/ocean/oceananigans.jl(1 hunks)
🔇 Additional comments (32)
experiments/ClimaEarth/components/ocean/oceananigans.jl (2)
171-172: Undefined variableair_density
This usage still referencesair_densityeven though it isn’t passed into the function or defined locally, which replicates a past review comment. Consider usingcsf.air_densityinstead.- Interfacer.update_field!(sim, Val(:air_density), air_density) + Interfacer.update_field!(sim, Val(:air_density), csf.air_density)
88-90:❓ Verification inconclusive
Verify the time-stepping logic
Passingt - sim.model.clock.timedirectly intoOceananigans.time_step!could cause unexpected behavior iftis less thansim.model.clock.time. Please verify that this aligns with the intended usage and potential negative or zero time steps are accounted for.
🏁 Script executed:
#!/bin/bash # Attempt to locate all references to `time_step!` calls in the code # to confirm that usage is consistent and no negative/zero step is introduced rg "time_step!\(.*\)"Length of output: 191
Action Item: Verify Handling of Negative/Zero Time Steps
The call to
Oceananigans.time_step!(sim.model, t - sim.model.clock.time)has been confirmed as the sole invocation in the codebase. However, passingt - sim.model.clock.timedirectly may yield negative or zero values iftis less than or equal tosim.model.clock.time. Please double-check that such values are either explicitly handled (e.g., with assertions or conditional checks) or natively managed by theOceananiganslibrary. If this behavior is unintended, consider adding safeguards to ensure that the time step remains strictly positive.experiments/ClimaEarth/Manifest.toml (13)
5-5: Project Hash Updated
The manifest’sproject_hashhas been updated to"49212e1dfe951689ca6571f5a37fc6f6430caa6f". This change is expected in a machine‐generated file when dependencies or the project state changes.
183-188: New Dependency: BFloat16s
A new dependency entry forBFloat16shas been added with the specified dependencies (LinearAlgebra,Printf, andRandom), git-tree SHA, UUID, and version"0.5.1". Please verify that this version is compatible with the rest of the ecosystem and that its inclusion supports the intended numerical computations.
202-206: New Dependency: BitFlags
The entry forBitFlagsis now present with its git-tree SHA, UUID, and version"0.1.9". Since BitFlags is a lightweight utility, ensure that its functionality and version meet the project’s needs without introducing conflicts.
262-267: CUDA Dependency Update
The[[deps.CUDA]]block now includes a comprehensive list of dependencies (includingBFloat16s,Crayons,GPUArrays, etc.) and is updated to version"5.7.1", with weak dependencies on"ChainRulesCore","EnzymeCore", and"SpecialFunctions". Please confirm that all these sub-dependencies correctly support GPU-accelerated operations and that no version conflicts arise across the toolchain.
274-278: CUDA_Driver_jll Version Update
This block updates theCUDA_Driver_jlldependency to version"0.12.1+1". Verify that this new version is consistent with the corresponding CUDA runtime and that it meets the hardware/driver requirements for your simulation workflow.
280-284: CUDA_Runtime_Discovery Update
TheCUDA_Runtime_Discoverydependency is now set to version"0.3.5". Make sure that this adjustment correctly detects the required runtime components and integrates smoothly with the rest of the CUDA-related libraries.
286-290: CUDA_Runtime_jll Update
The entry forCUDA_Runtime_jllhas been updated to version"0.16.1+0". This change should be verified to ensure that it properly matches the configurations set by the other CUDA packages and supports the runtime operations required by your simulation modules.
396-400: New/Updated ClimaOcean Dependency
The[[deps.ClimaOcean]]block has been added/updated with a robust list of dependencies (includingAdapt,CFTime,CUDA,ClimaSeaIce,Oceananigans, etc.) and is set to version"0.5.10". This dependency is central to the ocean simulation capability. Please ensure that its composition meets the requirements of the forthcomingOceananigansSimulationtemplate and that all sub-dependencies are compatible.
402-403: ClimaOcean Extension for Reactant
The manifest now specifies an extension:ClimaOceanReactantExt = "Reactant". Ensure that the Reactant package is appropriately integrated, as it may be utilized in downstream flux or coupling calculations within the simulation framework.
405-406: Weak Dependency on Reactant for ClimaOcean
A weak dependency onReactantis declared with its UUID"3c362404-f566-11ee-1572-e11a4b42c853". Confirm that this weak dependency does not force a hard version constraint and that it harmonizes with any optional simulation features that might leverage Reactant.
414-418: ClimaSeaIce Dependency Update
The[[deps.ClimaSeaIce]]block now includesOceananigansas one of its dependencies and is updated to version"0.2.5". This change is crucial if sea ice dynamics are to be coupled with the new ocean model. Verify that the integration between sea ice and ocean components is coherent.
476-480: CodecZlib Dependency Added
A new entry forCodecZlibhas been introduced, depending onTranscodingStreamsandZlib_jll, with version"0.7.8". Please check that this compression library meets the project’s requirements for data format handling and that it integrates well with other I/O components.
567-571: CompoundPeriods Dependency Added
The manifest now includes a dependency forCompoundPeriods(version"0.5.4"), which depends on"Dates". This addition should support any complex time-period computations required by the simulation template. Verify that its functionality aligns with the project’s temporal modeling needs.experiments/ClimaEarth/Manifest-v1.11.toml (17)
3-5: Project Hash UpdateThe project hash has been updated to
project_hash = "cbaa13594b44cc87ad732d5b6f1cb0f59d88893f"
which reflects a new project state. Please ensure this change matches the intended release state for this update.
184-189: New Dependency: BFloat16sThe block for the dependency BFloat16s (version 0.5.1) has been added/updated with the expected dependency list. This is likely required for enhanced numerical precision in GPU routines.
204-208: New Dependency: BitFlagsThe BitFlags dependency (version 0.1.9) is now included. Verify that its features (e.g. bitwise flag operations) will be used by some of the project’s components and that its version is correct.
265-271: Updated CUDA DependencyThe CUDA dependency now appears with a wide range of dependencies and has been updated to version 5.7.1. Note the inclusion of weak dependencies for ChainRulesCore, EnzymeCore, and SpecialFunctions. Please check that these versions work correctly with your GPU-based simulations and that there are no version conflicts in the broader dependency graph.
272-276: CUDA ExtensionsIn the CUDA extensions block, the following extensions have been added:
ChainRulesCoreExt = "ChainRulesCore"
EnzymeCoreExt = "EnzymeCore"
SpecialFunctionsExt = "SpecialFunctions"
Confirm that these extension entries are required for your code (for example, to support automatic differentiation and optimized mathematical routines).
283-288: CUDA Runtime Discovery DependencyThe CUDA_Runtime_Discovery dependency (version 0.3.5) has been updated. This module helps detect available CUDA runtimes. It would be good to verify on your target systems that this discovery mechanism performs as expected.
289-294: CUDA Runtime JLL DependencyThe CUDA_Runtime_jll now uses version 0.16.1+0. Make sure that this version is fully in line with your CUDA driver and runtime requirements across your supported platforms.
340-345: ClimaComms Weak DependenciesIn the ClimaComms block, a weak dependency on both CUDA and MPI has been specified. Please verify that these optional dependencies are desired for the typical deployment environments and do not inadvertently force unneeded inclusions in situations where, for example, MPI support is not available.
352-357: ClimaCore Weak Dependencies UpdatedThe ClimaCore dependency now lists weak dependencies on CUDA and Krylov (version updates appear in the weakdeps). Confirm that this update in weak dependencies is consistent with the design of your core simulation routines and does not introduce extra constraints.
405-407: ClimaOcean ExtensionsWithin the ClimaOcean block, the extension:
ClimaOceanReactantExt = "Reactant"
has been added. Confirm that this extension correctly enables any coupling or data transformation required when using the Reactant package.
408-410: ClimaOcean Weak DependencyA weak dependency for Reactant is specified under ClimaOcean. This indicates that while Reactant is not strictly required, it enhances functionality if available. Please ensure that this optional dependency aligns with your intended simulation coupling and that fallback behavior is in place when Reactant is absent.
417-422: New Dependency: ClimaSeaIceThe ClimaSeaIce dependency (version 0.2.5) has been added. Since this package supports ocean–ice interactions, verify that its API and version are compatible with the new ocean simulation template and that any exchange fields involving sea ice are properly addressed in subsequent development.
626-631: Dependency Update: CubicSplinesThe CubicSplines dependency has been updated to version 0.2.1. This update might affect any numerical routines using spline interpolation; make sure that these changes are intended and that all usages of the API are in line with the new version.
637-642: Dependency Update: DataDepsThe DataDeps dependency (version 0.7.13) has been updated. Please verify that the new version supports your project’s data acquisition and retrieval pipelines as expected.
643-648: Dependency Update: DataFramesThe DataFrames dependency now uses version 1.7.0. Given the widespread use of DataFrames in data manipulation, please ensure that this update does not break existing code (for example, by introducing incompatible API changes).
2968-2972: New Dependency: demumble_jllThe demumble_jll dependency has been updated to version 1.3.0+0. This package is likely related to symbol demangling in the compiled artifacts (especially relevant when dealing with CUDA or LLVM components). Please verify its necessity and compatibility within your toolchain.
2690-2694: New or Updated Dependency: TZJDataThe TZJData dependency has been updated to version 1.5.0+2025b. Since TZJData is usually used for handling time zone–related data, confirm that its version satisfies your project's time/date handling requirements.
| # TODO fill this in - | ||
| # compute F_turb_ρτxz, F_turb_ρτyz, F_shf, F_lhf, F_turb_moisture using Oceananigans.jl functions | ||
| # something like this: | ||
|
|
||
| fluxes = Oceananigans.compute_surface_fluxes(p, sim.model, Y, t, atmos_sim.integrator) |
There was a problem hiding this comment.
Placeholder references undefined variables
The code snippet referencing Oceananigans.compute_surface_fluxes(p, sim.model, Y, t, atmos_sim.integrator) invokes p, Y, and t which are not defined in this scope. This fragment likely cannot compile and needs proper parameters or references.
juliasloan25
force-pushed
the
js/ocean_template
branch
from
c27c6ed to
fecbe45
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
experiments/ClimaEarth/components/ocean/oceananigans.jl (2)
264-266:⚠️ Potential issueUndefined variables passed to
compute_surface_fluxes.
p,Y, andtare unused and not declared. Either remove them or ensure they are derived from your environment.- fluxes = Oceananigans.compute_surface_fluxes(p, sim.model, Y, t, atmos_sim.integrator) + # Possibly something like: + fluxes = Oceananigans.compute_surface_fluxes(sim.model, atmos_sim.integrator)
171-172:⚠️ Potential issueUndefined variable
air_density.This line references
air_densitydirectly, but it is never passed or defined. Usecsf.air_densityor rename the parameter in the function signature.-function FieldExchanger.update_sim!(sim::OceananigansSimulation, csf, turbulent_fluxes, area_fraction) Interfacer.update_field!(sim, Val(:air_density), air_density) +function FieldExchanger.update_sim!(sim::OceananigansSimulation, csf, turbulent_fluxes, area_fraction) + Interfacer.update_field!(sim, Val(:air_density), csf.air_density)
🧹 Nitpick comments (5)
experiments/ClimaEarth/components/ocean/oceananigans.jl (5)
6-20: Documentation covers high-level intent but could link to usage details.These docstrings do a decent job introducing the
OceananigansSimulationstruct and explaining fields. For clarity, consider adding short usage examples or references to relevant modules and functions, especially for new contributors or potential maintainers.
70-78: Initial conditions are set randomly.You use
rand()inTᵢandSᵢ, yielding non-reproducible setups. If reproducibility is desired (e.g., for tests), consider seeding random number generation or removing the random factor.
92-92: Reinitialization is not implemented.
Interfacer.reinit!is currently a no-op. If the model state or integrator time must be reset, ensure this is filled in to allow consistent restarts.
101-108: Consider implementing more surface fields.All fields except
:area_fractionreturnnothing. Some ocean schemes may need actual values (temperature, humidity, albedos, roughness, etc.). Be sure to fill these out once you have clarity on how the coupler and ocean model must share these.
289-291: Checkpointer not implemented.
Checkpointer.get_model_prog_stateraises an error, so restarts are currently unsupported. If you aim to integrate the restarts soon, consider implementing a partial state or an unstructured checkpoint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
experiments/ClimaEarth/Manifest-v1.11.toml(42 hunks)experiments/ClimaEarth/Manifest.toml(42 hunks)experiments/ClimaEarth/Project.toml(2 hunks)experiments/ClimaEarth/components/ocean/oceananigans.jl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- experiments/ClimaEarth/Project.toml
🔇 Additional comments (27)
experiments/ClimaEarth/components/ocean/oceananigans.jl (3)
1-5: Imports look good, but confirm dependencies.The imports from
Oceananigans,ClimaOcean,ClimaCoupler, andClimaCommsappear relevant. Just ensure all these modules are installed, properly versioned, and fully compatible.
194-200: No definitions forVal(:F_turb_ρτxz), etc.This function updates flux fields
F_turb_ρτxz,F_turb_ρτyz, etc., but correspondingInterfacer.update_field!overloads are missing. Properly define or handle them if your ocean model expects these flux fields.
83-91:❓ Verification inconclusive
Interfacer.step!usage.
step!(sim, t)callsOceananigans.time_step!(sim.model, t - sim.model.clock.time). Confirm that subtractingsim.model.clock.timefromtis indeed the correct way to get the time step increment. Also verify if this handles negative or zero increments gracefully.
🏁 Script executed:
#!/bin/bash # Searching for references to 'step!' usage to verify time stepping logic rg -A 5 'step!\(.*OceananigansSimulation'Length of output: 643
Confirm and Verify Time-Step Calculation and Edge Case Handling
In file
experiments/ClimaEarth/components/ocean/oceananigans.jl(lines 83–91), the implementation:Interfacer.step!(sim::OceananigansSimulation, t) = Oceananigans.time_step!(sim.model, t - sim.model.clock.time)computes the elapsed time (
dt) as the difference between the target timetand the current simulation time (sim.model.clock.time). Please ensure that:
- Correctness of Calculation: Subtracting
sim.model.clock.timefromtindeed represents the intended time step increment for this simulation.- Handling of Non-positive Increments: The implementation (or the underlying
Oceananigans.time_step!function) properly handles cases wheret - sim.model.clock.timeis zero or negative. If not, consider adding safeguards or explicit error handling.experiments/ClimaEarth/Manifest.toml (9)
5-5: Project Hash Update:
Theproject_hashon line 5 has been updated to"49212e1dfe951689ca6571f5a37fc6f6430caa6f", which reflects the overall change in the dependency graph. Please confirm that the manifest was regenerated in a clean environment and that the hash is consistent with all the new and updated dependencies.
183-188: Addition of BFloat16s Dependency:
A new dependency forBFloat16shas been introduced with its own dependencies (i.e.LinearAlgebra,Printf, andRandom) and version"0.5.1". This should support any operations in the simulation that require BFloat16 numerical types. Verify that its inclusion is intentional and that its version is compatible with the broader dependency ecosystem.
202-206: Addition of BitFlags Dependency:
TheBitFlagspackage (version"0.1.9") has been added. Please confirm that this package is needed—perhaps for efficient management of flag values—and that it integrates well with the other simulation components.
262-273: New CUDA Package Block:
A comprehensive new block for theCUDApackage has been introduced (version"5.7.1") with a long dependency list and weak dependencies onChainRulesCore,EnzymeCore, andSpecialFunctions. This update is critical for GPU-accelerated parts of the simulation (e.g. in the upcoming OceananigansSimulation). Please verify that:
- All listed dependencies match the project’s GPU requirements.
- The weak dependencies resolve correctly in your environment.
- There are no version conflicts with other GPU-related packages in your dependency tree.
274-279: CUDA_Driver_jll Addition:
TheCUDA_Driver_jllpackage (version"0.12.1+1") has been added. This is necessary for interfacing with the system’s CUDA drivers. Make sure that the installed driver on your system is compatible with this version, and that it does not introduce conflicts with the mainCUDApackage functionality.
280-285: CUDA_Runtime_Discovery Inclusion:
The new dependencyCUDA_Runtime_Discovery(version"0.3.5") is aimed at discovering CUDA runtime libraries at execution time. This addition should help in configuring the runtime environment correctly. Verify that it integrates seamlessly with the other CUDA components.
286-291: CUDA_Runtime_jll Update:
TheCUDA_Runtime_jllpackage (version"0.16.1+0") has been added, ensuring that the CUDA runtime components are available to the project. It is important that the runtime, driver, and main CUDA package versions are synchronized. Please confirm that this version is fully compatible with the rest of your CUDA setup.
396-401: Inclusion of ClimaOcean Dependency:
The new dependencyClimaOcean(version"0.5.10") is critical for enabling ocean simulation capabilities. It brings in several packages—includingOceananigans—that will support the development of theOceananigansSimulationtemplate. Please ensure that:
- All the sub-dependencies (e.g.,
Oceananigans,CubicSplines,NCDatasets, etc.) are compatible with your overall ClimaCoupler ecosystem.- Any integration tests regarding ocean simulations are updated to use this dependency.
402-407: ClimaOcean Extensions and Weak Dependencies:
The extensions block forClimaOceannow definesClimaOceanReactantExtand marksReactantas a weak dependency. This appears to be linked to the simulation’s need for coupling reactive components. Make sure that theReactantpackage—with UUID"3c362404-f566-11ee-1572-e11a4b42c853"—meets your integration requirements and that any OceananigansSimulation code referring to these components is updated accordingly.experiments/ClimaEarth/Manifest-v1.11.toml (15)
5-5: Project Hash Update Detected
Theproject_hashhas been updated to"cbaa13594b44cc87ad732d5b6f1cb0f59d88893f". Please verify that this new hash accurately reflects the current state of all project dependencies.
185-189: New Dependency Addition: BFloat16s
The dependency forBFloat16shas been added with version"0.5.1", including its own dependencies:LinearAlgebra,Printf, andRandom. Confirm that the git-tree-sha1 and the version are as expected for integration.
204-207: New Dependency Addition: BitFlags
A new dependency forBitFlags(version"0.1.9") has been introduced. Please check that its git-tree-sha1 and UUID match the release details and that it is compatible with the rest of the manifest.
265-269: CUDA Dependency Update – Main Block
TheCUDAdependency now lists a comprehensive set of dependencies and has been updated to version"5.7.1". Verify that the inclusion of packages such asBFloat16s,CEnum, and others (along with weak dependencies onChainRulesCore,EnzymeCore, andSpecialFunctions) is intentional and that they are mutually compatible.
272-276: CUDA Extensions Configuration
The extension settings for CUDA now includeChainRulesCoreExt,EnzymeCoreExt, andSpecialFunctionsExt. Please confirm that these extensions correctly map to their parent packages and support the intended GPU computing and simulation features.
278-282: CUDA_Driver_jll Dependency Addition
TheCUDA_Driver_jlldependency has been added with version"0.12.1+1". Verify that the listed dependencies and git-tree-sha1 are current and meet the project’s driver requirements for CUDA interoperability.
284-288: CUDA_Runtime_Discovery Dependency Added
The dependencyCUDA_Runtime_Discovery(version"0.3.5") is now included. Please ensure this module’s lightweight dependency onLibdland its version align with the runtime discovery needs for CUDA.
289-293: CUDA_Runtime_jll Dependency Update
TheCUDA_Runtime_jlldependency has been updated to version"0.16.1+0"with its corresponding dependencies. Verify that this update integrates correctly with the CUDA driver and overall GPU runtime environment.
399-403: ClimaOcean Dependency Introduced
A new dependencyClimaOceanhas been added with version"0.5.10". This dependency is essential for ocean modeling and aligns with the new OceananigansSimulation template. Please ensure that its extensive dependency list (including items likeCUDA,ClimaSeaIce, andOceananigans) is correct and that there are no conflicts with other dependencies.
405-407: ClimaOcean Extensions Configuration
WithinClimaOcean, an extension namedClimaOceanReactantExthas been specified. Confirm that this extension correctly linksClimaOceanwith theReactantpackage and supports the intended simulation coupling.
408-410: ClimaOcean Weak Dependencies Setup
The weak dependency forClimaOceanincludesReactant. Please check that markingReactantas a weak dependency is appropriate for optional functionality and does not lead to unexpected runtime issues.
1905-1909: New Dependency Addition: Oceananigans (Main Block)
The dependencyOceananiganshas been added with version"0.96.12". Its extensive dependency list is critical for ocean simulation functionality. Please verify that the git-tree-sha1, UUID, and version match the expected release and that it integrates smoothly with the overarching ClimaCoupler ecosystem.
1910-1916: Oceananigans Extensions Configuration
The extension configuration forOceananigansnow provides additional functionality throughOceananigansEnzymeExt,OceananigansMakieExt,OceananigansMetalExt,OceananigansNCDatasetsExt, andOceananigansReactantExt. Confirm that these extensions are implemented as intended and offer the necessary hooks for performance, visualization, and optional coupling.
1917-1925: Oceananigans Weak Dependencies Setup
The weak dependencies forOceananigansinclude packages such asConstructionBase,Enzyme,Makie,MakieCore,Metal,NCDatasets, andReactant. Please ensure that these are correctly set as optional integrations, providing flexibility without enforcing strict version requirements.
2968-2972: demumble_jll Dependency Update
The dependencydemumble_jllhas been added with version"1.3.0+0". Verify that its configuration, including the listed dependencies and git-tree-sha1, meets the project’s needs for demangling or debugging support as required by the simulation modules.
| struct OceananigansSimulation{M, I, A} <: Interfacer.OceanModelSimulation | ||
| sim::M | ||
| area_fraction::A | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confer type parameter usage.
The struct parameters {M, I, A} serve to define the ocean simulation model, integrator, and area fraction type. Verify if the I is still necessary since the constructor doesn't reference an integrator parameter.
🏁 Script executed:
#!/bin/bash
# Searching for references to the `I` type parameter in the codebase
rg --context 5 'OceananigansSimulation\{.*I.*\}'Length of output: 1907
Unused Type Parameter in OceananigansSimulation
In experiments/ClimaEarth/components/ocean/oceananigans.jl (lines 17–20), the struct still declares the type parameter I, but it isn’t referenced in any field or constructor. Please evaluate whether this parameter serves a purpose elsewhere (for example, if intended for future integration or required by the parent type) and either remove it or update the struct to utilize it accordingly.
| function OceananigansSimulation( | ||
| ::Type{FT}, # TODO decide which arguments we want here | ||
| dt::TT, | ||
| tspan::Tuple{TT, TT}, | ||
| start_date::Dates.DateTime, | ||
| output_dir::String, | ||
| area_fraction, | ||
| comms_ctx = ClimaComms.context(), | ||
| ) where {FT, TT <: Union{Float64, ITime}} | ||
| # TODO fill this out | ||
| arch = if comms_ctx.device isa ClimaComms.CUDADevice | ||
| Oceananigans.GPU() | ||
| else | ||
| Oceananigans.CPU() | ||
| end | ||
|
|
||
| # Set up ocean grid | ||
| Nx = 360 | ||
| Ny = 170 | ||
| Nz = 30 | ||
| z_faces = Ocean.exponential_z_faces(; Nz, h = 30, depth = 6000) | ||
|
|
||
| # TODO how to specify FT? | ||
| grid = Oceananigans.LatitudeLongitudeGrid( | ||
| arch; | ||
| size = (Nx, Ny, Nz), | ||
| longitude = (0, 360), | ||
| latitude = (-85, 85), | ||
| z = z_faces, | ||
| halo = (7, 7, 7), | ||
| ) | ||
|
|
||
| # Choose parameterizations | ||
| momentum_advection = Oceananigans.WENOVectorInvariant(order = 5) | ||
| tracer_advection = Oceananigans.WENO(order = 5) | ||
| free_surface = Oceananigans.SplitExplicitFreeSurface(grid; substeps = 30) | ||
|
|
||
| # Create ocean simulation | ||
| ocean = Ocean.ocean_simulation(grid; momentum_advection, tracer_advection, free_surface, warn = false) | ||
|
|
||
|
|
||
| # Set up initial conditions for temperature and salinity | ||
| Tᵢ(λ, φ, z) = 30 * (1 - tanh((abs(φ) - 30) / 5)) / 2 + rand() | ||
| Sᵢ(λ, φ, z) = 30 - 5e-3 * z + rand() | ||
| Oceananigans.set!(ocean.model, T = Tᵢ, S = Sᵢ) | ||
|
|
||
| # TODO do we want to store sim or model? | ||
| return OceananigansSimulation(ocean, area_fraction) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The constructor is incomplete.
- It mentions
# TODO fill this out, but the function already instantiates an ocean simulation viaOcean.ocean_simulation(...). - Unused parameters:
start_date,output_dir, andtspanare not applied anywhere. - If you plan to incorporate domain decomposition or more sophisticated initial conditions, ensure these placeholders eventually get used.
function OceananigansSimulation(
::Type{FT},
dt::TT,
tspan::Tuple{TT, TT},
start_date::Dates.DateTime,
output_dir::String,
area_fraction,
comms_ctx = ClimaComms.context(),
) where {FT, TT <: Union{Float64, ITime}}
# TODO fill this out
+ # e.g. possibly use 'start_date' or 'output_dir' to configure checkpointing:
+ # setup_output(output_dir, start_date)📝 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.
| function OceananigansSimulation( | |
| ::Type{FT}, # TODO decide which arguments we want here | |
| dt::TT, | |
| tspan::Tuple{TT, TT}, | |
| start_date::Dates.DateTime, | |
| output_dir::String, | |
| area_fraction, | |
| comms_ctx = ClimaComms.context(), | |
| ) where {FT, TT <: Union{Float64, ITime}} | |
| # TODO fill this out | |
| arch = if comms_ctx.device isa ClimaComms.CUDADevice | |
| Oceananigans.GPU() | |
| else | |
| Oceananigans.CPU() | |
| end | |
| # Set up ocean grid | |
| Nx = 360 | |
| Ny = 170 | |
| Nz = 30 | |
| z_faces = Ocean.exponential_z_faces(; Nz, h = 30, depth = 6000) | |
| # TODO how to specify FT? | |
| grid = Oceananigans.LatitudeLongitudeGrid( | |
| arch; | |
| size = (Nx, Ny, Nz), | |
| longitude = (0, 360), | |
| latitude = (-85, 85), | |
| z = z_faces, | |
| halo = (7, 7, 7), | |
| ) | |
| # Choose parameterizations | |
| momentum_advection = Oceananigans.WENOVectorInvariant(order = 5) | |
| tracer_advection = Oceananigans.WENO(order = 5) | |
| free_surface = Oceananigans.SplitExplicitFreeSurface(grid; substeps = 30) | |
| # Create ocean simulation | |
| ocean = Ocean.ocean_simulation(grid; momentum_advection, tracer_advection, free_surface, warn = false) | |
| # Set up initial conditions for temperature and salinity | |
| Tᵢ(λ, φ, z) = 30 * (1 - tanh((abs(φ) - 30) / 5)) / 2 + rand() | |
| Sᵢ(λ, φ, z) = 30 - 5e-3 * z + rand() | |
| Oceananigans.set!(ocean.model, T = Tᵢ, S = Sᵢ) | |
| # TODO do we want to store sim or model? | |
| return OceananigansSimulation(ocean, area_fraction) | |
| end | |
| function OceananigansSimulation( | |
| ::Type{FT}, # TODO decide which arguments we want here | |
| dt::TT, | |
| tspan::Tuple{TT, TT}, | |
| start_date::Dates.DateTime, | |
| output_dir::String, | |
| area_fraction, | |
| comms_ctx = ClimaComms.context(), | |
| ) where {FT, TT <: Union{Float64, ITime}} | |
| # TODO fill this out | |
| # e.g. possibly use 'start_date' or 'output_dir' to configure checkpointing: | |
| # setup_output(output_dir, start_date) | |
| arch = if comms_ctx.device isa ClimaComms.CUDADevice | |
| Oceananigans.GPU() | |
| else | |
| Oceananigans.CPU() | |
| end | |
| # Set up ocean grid | |
| Nx = 360 | |
| Ny = 170 | |
| Nz = 30 | |
| z_faces = Ocean.exponential_z_faces(; Nz, h = 30, depth = 6000) | |
| # TODO how to specify FT? | |
| grid = Oceananigans.LatitudeLongitudeGrid( | |
| arch; | |
| size = (Nx, Ny, Nz), | |
| longitude = (0, 360), | |
| latitude = (-85, 85), | |
| z = z_faces, | |
| halo = (7, 7, 7), | |
| ) | |
| # Choose parameterizations | |
| momentum_advection = Oceananigans.WENOVectorInvariant(order = 5) | |
| tracer_advection = Oceananigans.WENO(order = 5) | |
| free_surface = Oceananigans.SplitExplicitFreeSurface(grid; substeps = 30) | |
| # Create ocean simulation | |
| ocean = Ocean.ocean_simulation(grid; momentum_advection, tracer_advection, free_surface, warn = false) | |
| # Set up initial conditions for temperature and salinity | |
| Tᵢ(λ, φ, z) = 30 * (1 - tanh((abs(φ) - 30) / 5)) / 2 + rand() | |
| Sᵢ(λ, φ, z) = 30 - 5e-3 * z + rand() | |
| Oceananigans.set!(ocean.model, T = Tᵢ, S = Sᵢ) | |
| # TODO do we want to store sim or model? | |
| return OceananigansSimulation(ocean, area_fraction) | |
| end |
Purpose
Add a file
oceananigans.jlto implement theOceananigansSimulationin.To-do
Content
Summary by CodeRabbit