Split setup_and_run in multiple steps
#1251
Conversation
|
Warning Rate limit exceeded@Sbozzolo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThe changes refactor the simulation lifecycle by splitting the monolithic Changes
🪧 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 (
|
Sbozzolo
force-pushed
the
gb/step
branch
2 times, most recently
from
faf50f9 to
74e480e
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: 0
🧹 Nitpick comments (3)
src/Checkpointer.jl (1)
162-164: Improved file handling with do-block syntaxRestructured
restart_model_cache!to use adoblock withJLD2.jldopen, which ensures proper file closure and improves code readability.NEWS.md (2)
11-18: Comprehensive documentation of structural changesThe documentation clearly explains how
setup_and_runwas split into three separate functions with distinct responsibilities, as well as the addition of the newstep!function for debugging purposes.However, there's a minor grammatical issue:
-`setup_and_run` was split in three functions: +`setup_and_run` was split into three functions:🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: The preposition “into” seems more likely in this position than the preposition “in”.
Context: ...l/pull/1251)setup_and_runwas split in three functions: -CoupledSimulation,...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_INTO)
24-30: Well-documented cleanup changesThe documentation provides a clear bulleted list of cleanup tasks, making it easy to understand the scope of the changes.
However, there's a minor grammatical issue in the second bullet point:
-`SurfaceModelSimulation`s no longer have a `domain` field, which were unused. +`SurfaceModelSimulation`s no longer have a `domain` field, which was unused.🧰 Tools
🪛 LanguageTool
[grammar] ~27-~27: There seems to be a noun/verb agreement error. Did you mean “has” or “had”?
Context: .... -SurfaceModelSimulations no longer have adomainfield, which were unused. - ...(SINGULAR_NOUN_VERB_AGREEMENT)
🪛 markdownlint-cli2 (0.17.2)
24-24: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
NEWS.md(2 hunks)docs/src/timemanager.md(0 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(2 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(3 hunks)experiments/ClimaEarth/components/ocean/eisenman_seaice.jl(2 hunks)experiments/ClimaEarth/components/ocean/prescr_ocean.jl(3 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(6 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(2 hunks)experiments/ClimaEarth/components/shared/restore.jl(1 hunks)experiments/ClimaEarth/run_amip.jl(1 hunks)experiments/ClimaEarth/run_cloudless_aquaplanet.jl(4 hunks)experiments/ClimaEarth/run_cloudy_aquaplanet.jl(4 hunks)experiments/ClimaEarth/run_cloudy_slabplanet.jl(6 hunks)experiments/ClimaEarth/run_dry_held_suarez.jl(2 hunks)experiments/ClimaEarth/run_moist_held_suarez.jl(4 hunks)experiments/ClimaEarth/setup_run.jl(14 hunks)experiments/ClimaEarth/test/component_model_tests/prescr_ocean_tests.jl(2 hunks)experiments/ClimaEarth/test/component_model_tests/prescr_seaice_tests.jl(1 hunks)experiments/ClimaEarth/test/component_model_tests/slab_ocean_tests.jl(1 hunks)experiments/ClimaEarth/test/debug_plots_tests.jl(1 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(7 hunks)experiments/ClimaEarth/user_io/coupler_diagnostics.jl(2 hunks)experiments/ClimaEarth/user_io/postprocessing.jl(2 hunks)src/Checkpointer.jl(4 hunks)src/Interfacer.jl(5 hunks)src/TimeManager.jl(1 hunks)test/conservation_checker_tests.jl(3 hunks)test/field_exchanger_tests.jl(1 hunks)test/flux_calculator_tests.jl(6 hunks)test/interfacer_tests.jl(3 hunks)test/runtests.jl(0 hunks)test/time_manager_tests.jl(0 hunks)
💤 Files with no reviewable changes (3)
- test/runtests.jl
- docs/src/timemanager.md
- test/time_manager_tests.jl
🧰 Additional context used
🪛 LanguageTool
NEWS.md
[uncategorized] ~11-~11: The preposition “into” seems more likely in this position than the preposition “in”.
Context: ...l/pull/1251) setup_and_run was split in three functions: - CoupledSimulation,...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_INTO)
[grammar] ~27-~27: There seems to be a noun/verb agreement error. Did you mean “has” or “had”?
Context: .... - SurfaceModelSimulations no longer have a domain field, which were unused. - ...
(SINGULAR_NOUN_VERB_AGREEMENT)
🪛 markdownlint-cli2 (0.17.2)
NEWS.md
9-9: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
24-24: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
32-32: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ 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: ci 1.10 - macOS-latest
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: test (1.11)
- GitHub Check: docbuild
🔇 Additional comments (110)
experiments/ClimaEarth/test/debug_plots_tests.jl (1)
78-79: Time step tracking parameter added to support new simulation lifecycle.Adding
Ref(Int(0))as the time step parameter aligns with the PR objective of splittingsetup_and_run. This parameter enables tracking the current simulation step, which is essential for the newstep!function that allows executing a single coupling step at a time.experiments/ClimaEarth/test/component_model_tests/prescr_seaice_tests.jl (1)
103-103: Constructor signature simplified by removing redundant domain parameter.The removal of the second
nothingparameter simplifies thePrescribedIceSimulationconstructor, aligning with the PR's objective to enhance the structure of ClimaEarth. This change corresponds with the simplification of the underlying structure where thedomain::Dfield was removed.experiments/ClimaEarth/test/component_model_tests/prescr_ocean_tests.jl (1)
21-21: Renamingdate0tostart_datefor improved clarity.This renaming standardizes date variable naming across the codebase, making the code more readable and consistent. The change has been applied consistently at declaration, constructor call, and when used as a reference date parameter.
Also applies to: 28-28, 45-45
test/field_exchanger_tests.jl (1)
158-158: Time step tracking parameter added to support new simulation lifecycle.Adding
Ref(Int(0))as the time step parameter aligns with the PR objective of splittingsetup_and_run. This parameter enables tracking the current simulation step, which is essential for the newstep!function that allows executing a single coupling step at a time.experiments/ClimaEarth/components/shared/restore.jl (1)
78-78: Improved error message detail inrestore!methodThe error message has been enhanced to show the actual values that caused the inconsistency, which makes debugging easier when immutable values don't match.
experiments/ClimaEarth/user_io/coupler_diagnostics.jl (4)
6-6: Updated function documentation to reflect parameter changeThe documentation string now correctly references
diag_calendar_dtinstead ofcalendar_dt, which aligns with the new parameter name in the function signature.
14-14: Addedcoupled_dtparameter to function signatureThis change to
coupler_diagnostics_setupadds a new parametercoupled_dtwhich enables passing the simulation time step to the diagnostics setup. This supports the refactoring goal of better separation between simulation setup and execution.
17-17: Updated parameter name fromcalendar_dttodiag_calendar_dtThe parameter has been renamed to more clearly indicate its purpose for diagnostics scheduling.
51-51: Passedcoupled_dtto diagnostics handlerThe
coupled_dtparameter is now used when creating the diagnostics handler, ensuring proper time step configuration for diagnostics. This change is part of improving the separation between setup and running phases.experiments/ClimaEarth/test/component_model_tests/slab_ocean_tests.jl (1)
24-24: SimplifiedSlabOceanSimulationinstantiationThe constructor call has been simplified by removing the second argument (previously
nothing). This change aligns with the broader refactoring to improve the simulation object's interface.experiments/ClimaEarth/run_amip.jl (2)
46-48: Replacedsetup_and_runwith separatedCoupledSimulationandrun!callsThis change implements the core refactoring goal of splitting the simulation setup from its execution. Now users can create a simulation object without immediately running it, which enables more interactive workflows and better debugging capabilities.
49-52: Added explicit postprocessing stepThe postprocessing has been separated into its own function call, completing the three-part workflow (setup → run → postprocess) outlined in the PR objectives. The retrieval of the
conservation_softfailparameter shows proper integration with the existing configuration system.The TODO comment about removing the
conservation_softfailoption suggests this may be refactored further in future updates.experiments/ClimaEarth/components/ocean/prescr_ocean.jl (3)
37-37: Parameter renaming looks goodThe parameter name change from
date0tostart_datein the documentation is consistent with the broader refactoring effort to improve naming clarity.
54-54: Consistent parameter renaming in function signatureThe parameter name change from
date0tostart_datein the function signature matches the documentation update above, maintaining consistency.
80-80: Properly updated reference in TimeVaryingInput initializationThe reference to
start_datein theSST_timevaryinginputcreation correctly uses the renamed parameter, ensuring consistent date handling.test/interfacer_tests.jl (3)
4-6: Added necessary importsThe added imports for
ClimaCommsandDatesalong with the conditional backend import are necessary for the updated functionality.
31-31: Simplified get_field implementationThe function implementation has been simplified to directly return
CC.Spaces.undertype(sim.space)(2)rather than calculating the float type based on the mesh domain.
43-43: Added time tracking parameter to CoupledSimulationThe addition of
Ref(Int(0))as the time tracking parameter to theCoupledSimulationconstructor is consistent with the PR's objective of splitting thesetup_and_runfunction and enabling step-by-step simulation execution.src/Checkpointer.jl (3)
11-11: Optimized importsThe import statement has been refined to only import the specific needed functions
ITimeandsecondsfromClimaUtilities.TimeManager.
103-103: Updated time calculationTime calculation now uses
cs.t[]directly instead of computing from two epoch timestamps. This simplification aligns with the new structure where time is tracked via a reference.
192-198: Improved file handling consistencySimilar to the above change,
restart_coupler_fields!now uses adoblock withJLD2.jldopenfor better resource management and consistent style.experiments/ClimaEarth/user_io/arg_parsing.jl (6)
71-71: Renamed date parameter for clarityParameter name change from
date0tostart_dateimproves code clarity by using a more descriptive name.
75-77: Updated ITime initialization with start_dateAll uses of
date0in the ITime constructor have been replaced withstart_date, maintaining consistency with the parameter renaming.
102-102: Renamed calendar_dt to diag_calendar_dt for specificityThe variable renaming from
calendar_dttodiag_calendar_dtmakes its purpose clearer - it specifically relates to diagnostics rather than general calendar handling.Also applies to: 137-137
198-211: Consistent variable renaming throughout get_diag_periodAll instances of
calendar_dthave been renamed todiag_calendar_dtwithin the function, maintaining consistency with the broader renaming pattern.
212-212: Updated return value to match renamed variableReturn statement correctly uses the renamed
diag_calendar_dtvariable.
286-286: Consistent variable naming in add_extra_diagnostics!All occurrences of the diagnostic calendar variable now use the more specific name
diag_calendar_dtfor better clarity.Also applies to: 289-289, 299-299
src/Interfacer.jl (5)
11-11: Updated imports to support new time handling.The imports have been updated to include
Datesand specifically importITimeanddatefromClimaUtilities.TimeManager. These changes support the new time management approach in theCoupledSimulationstruct.Also applies to: 14-14
59-59: Improved time management in CoupledSimulation struct.The struct has been updated with:
- A new type parameter
TTfor time tracking- Renaming
dates::Dtostart_date::Dfor clarity- Adding a new field
t::TTto track the current simulation timeThese changes provide better time tracking capabilities and make the time-related fields more explicit and easier to understand.
Also applies to: 68-69, 74-74
83-83: Simplified the CoupledSimulation constructor.The constructor has been simplified to use type inference from the arguments. This is a good improvement that reduces boilerplate code while maintaining type safety.
92-92: Updated display of current date in Base.show.The method now uses
current_date(sim)instead of directly accessing date fields, providing a more consistent API for retrieving the current simulation date.
96-104: Added new current_date function for improved time handling.This function centralizes the logic for retrieving the current date of the simulation, handling both
ITimeand other time types appropriately. This is a nice improvement for API consistency and makes the code more maintainable.src/TimeManager.jl (1)
68-74: Simplified callback triggering with improved time access.The
maybe_trigger_callbackfunction has been simplified by removing the explicittparameter. It now retrieves the time directly from the simulation object usingt = cs.t[]. This change makes the function more straightforward and aligns with the new time management approach inCoupledSimulation.experiments/ClimaEarth/user_io/postprocessing.jl (2)
15-15: Simplified variable extraction from postprocessing_vars.The function now only extracts the essential
conservation_softfailparameter, removing unnecessary extraction ofoutput_default_diagnosticsandt_end. This simplification reduces coupling between the components.
32-39: Enhanced leaderboard plotting with data-driven approach.The leaderboard plotting logic has been improved to:
- Check for the availability of "pfull" in the simulation directory
- Extract time information from the simulation directory itself
- Use the actual end time to determine if enough data is available for the leaderboard
This data-driven approach is more robust as it relies on the actual simulation data rather than passed parameters.
experiments/ClimaEarth/components/land/climaland_integrated.jl (2)
15-15: Streamlined ClimaLandSimulation struct by removing domain dependency.The
Dtype parameter has been removed from the struct definition, simplifying the type signature. This change reduces coupling between the simulation and specific domain implementations, making the code more flexible.Also applies to: 27-28
203-203: Simplified ClimaLandSimulation constructor return.The constructor no longer includes the domain in the return statement, aligning with the struct changes. This is part of the broader effort to reduce domain dependencies across simulation structures.
experiments/ClimaEarth/components/ocean/slab_ocean.jl (3)
10-10: Structure signature updated for better designThe structure signature has been simplified by removing the domain type parameter
D, aligning with other simulation types in the codebase. This change follows the PR's goal of streamlining domain management across simulation objects.
17-17: Structure declaration properly updatedThe structure declaration correctly implements the signature change, removing the
Dtype parameter while maintaining the inheritance relationship withInterfacer.OceanModelSimulation.
101-101: Constructor call updated correctlyThe constructor call has been properly updated to remove the
spaceparameter, aligning with the removal of the domain field from the structure. This maintains consistency with the new simplified design.experiments/ClimaEarth/run_moist_held_suarez.jl (5)
222-223: Start date handling simplifiedThe
date0variable has been removed and replaced with direct usage ofstart_date, simplifying the date management. This is part of the PR's goal to enhance clarity in time-related data management.
228-228: Schedule checkpoint initialization streamlinedThe
schedule_checkpointnow directly referencesstart_dateinstead ofdate0, creating a more direct and streamlined approach to checkpoint scheduling.
248-249: CoupledSimulation construction improvedThe constructor now uses
Ref(start_date)instead of a dates dictionary and adds a reference to the current simulation time withRef(tspan[1]). This change follows the PR's goal of enhancing the clarity of time management by using a more direct reference.Also applies to: 254-254
311-312: Time update mechanism simplifiedThe mechanism for updating the current time has been changed from updating a date structure (
cs.dates.date[]) to a more straightforward time representation (cs.t[] = t). This aligns with the PR's goal of simplifying time management.
329-329: Callback trigger updated for new time managementThe callback trigger has been updated to not pass the time parameter
t, reflecting the change in how time is managed internally in theCoupledSimulationobject.experiments/ClimaEarth/run_dry_held_suarez.jl (5)
185-186: Start date handling simplifiedSimilar to the changes in
run_moist_held_suarez.jl, thedate0variable has been removed and replaced with direct usage ofstart_date, simplifying the date management.
191-191: Schedule checkpoint initialization streamlinedThe
schedule_checkpointnow directly referencesstart_dateinstead ofdate0, creating a more direct and streamlined approach to checkpoint scheduling.
198-198: CoupledSimulation construction improvedThe constructor now uses
Ref(start_date)instead of a dates dictionary and adds a reference to the current simulation time withRef(tspan[1]). This matches the changes made inrun_moist_held_suarez.jlfor consistency.Also applies to: 204-204
237-238: Time update mechanism simplifiedThe mechanism for updating the current time has been changed from updating a date structure to a more straightforward time representation (
cs.t[] = t). This aligns with the PR's goal of simplifying time management.
246-246: Callback trigger updated for new time managementThe callback trigger has been updated to not pass the time parameter
t, reflecting the change in how time is managed internally in theCoupledSimulationobject.NEWS.md (2)
9-9: Clear PR reference in headingThe heading properly references the PR number and title, making it easy to track the changes back to their source.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
21-22: Clear explanation of variable renamingThe documentation clearly explains the renaming of
calendar_dttodiag_calendar_dt, providing the reasoning behind the change - to make it clearer that it refers to diagnostics.test/flux_calculator_tests.jl (7)
66-69: Type parameter simplification looks goodGood refactoring to remove the domain type parameter
Dand field fromTestOcean. This simplifies the type definition and aligns with the PR goal of making the component structure more modular.
96-99: Type parameter simplification looks goodGood refactoring to remove the domain type parameter
Dand field fromDummySurfaceSimulation3. This is consistent with the changes made to other simulation structures.
193-193: Constructor update correctly reflects type changesGood update to the
TestOceanconstructor call to reflect the removal of the domain field, passing only the necessary parameters.
196-196: Consistent constructor call updateThis change correctly matches the updated type structure for
TestOcean.
272-272: Constructor update correctly reflects type changesThe constructor call for
DummySurfaceSimulation3has been properly updated to reflect the type parameter changes.
283-284: Constructor parameter structure updated correctlyUpdated initialization of
surface_simcorrectly reflects the removal of the domain parameter fromDummySurfaceSimulation3.
305-305: Using Ref for time trackingGood change to use
Ref(Int(0))for time tracking, which aligns with the modular approach in the PR. This facilitates easier state management in the split simulation workflow.experiments/ClimaEarth/components/ocean/eisenman_seaice.jl (3)
10-11: Updated docstring correctly documents type parametersGood update to the docstring to reflect the removal of the domain type parameter, maintaining consistency between code and documentation.
18-21: Type simplification consistent with PR objectivesThe removal of the domain type parameter and field from
EisenmanIceSimulationis consistent with the changes made to other simulation structures, supporting the PR's goal of creating a more modular codebase.
89-89: Constructor call updated correctlyThe constructor call for
EisenmanIceSimulationhas been properly updated to remove the domain parameter, reflecting the structural changes to the type.experiments/ClimaEarth/run_cloudless_aquaplanet.jl (7)
216-217: Simplified date handlingGood simplification of date handling by directly initializing
start_dateas aDateTimeobject instead of using an intermediatedate0variable. This improves code clarity.
222-223: Schedule parameter updateCorrectly updated
schedule_checkpointto use the simplified date handling approach, passingstart_datedirectly to the schedule constructor.
225-226: Schedule parameter updateCorrectly updated
schedule_albedoto use the simplified date handling approach, consistent with the changes toschedule_checkpoint.
249-250: CoupledSimulation structure updated for improved state managementGood update to the
CoupledSimulationstructure usingRef(start_date)for date tracking andRef(tspan[1])for time tracking. This approach allows for better state management in the modular simulation workflow.Also applies to: 255-256
312-314: Simplified time trackingGood simplification of time tracking by directly updating
cs.t[]instead of using a more complex date structure. This change makes the code more straightforward and easier to understand.
318-319: Simplified callback triggerUpdated the water albedo callback trigger to rely solely on the
csobject without requiring the time parameter, which streamlines the callback interface.
333-334: Simplified callback triggerUpdated the checkpoint callback trigger to use the simplified interface, consistent with the water albedo callback changes.
experiments/ClimaEarth/run_cloudy_aquaplanet.jl (6)
235-236: Simplified date handlingGood simplification of date handling by directly initializing
start_dateas aDateTimeobject. This change is consistent with the modifications in other files and improves code clarity.
241-242: Schedule parameter updateCorrectly updated
schedule_checkpointto use the simplified date handling approach, passingstart_datedirectly to the schedule constructor.
244-245: Schedule parameter updateCorrectly updated
schedule_albedoto use the simplified date handling approach, consistent with the changes toschedule_checkpoint.
267-268: CoupledSimulation structure updated for improved state managementGood update to the
CoupledSimulationstructure usingRef(start_date)for date tracking andRef(tspan[1])for time tracking. This approach allows for better state management in the modular simulation workflow.Also applies to: 273-274
330-331: Simplified time trackingGood simplification of time tracking by directly updating
cs.t[]instead of using a more complex date structure. This change makes the code more straightforward and easier to understand.
348-349: Simplified callback triggerUpdated the checkpoint callback trigger to use the simplified interface, which streamlines the callback system.
experiments/ClimaEarth/components/ocean/prescr_seaice.jl (6)
14-14: Docstring reflects updated type parameters.
It now denotes{P, I}instead of{P, D, I}. Make sure the docstring elsewhere clarifies that thedomainparameter was removed for simplicity.
66-66:start_dateparameter added to constructor.
This aligns with the broader transition fromdate0tostart_date. Looks good.
88-88: Repeated addition ofstart_dateparameter.
Consistent with the removal ofdate0usage. No issues found.
108-108: Settingreference_datetostart_date.
The re-labeled argument enhances clarity and consistency. Good change.
148-148: Constructor call updated to reflect the new signature.
The removal of thedomainargument here matches the struct changes. Implementation is correct.
30-30:❓ Verification inconclusive
Struct simplified by removing the domain parameter.
RemovingDfrom the type signature is consistent with the refactoring. Ensure all references to the previousDtype are removed or updated across the codebase.Run the following script to confirm no references to a
domainfield remain:
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'struct PrescribedIceSimulation { domain: $_, $$$ }'Length of output: 73
Below is an additional check to further verify that no leftover references to a
domain:field exist. Please run the following script to search all Julia files for any mention of “domain:”:Once you confirm that this search returns no results, you can be assured that all remnants of the removed
Ddomain parameter have been successfully eliminated. Here’s the final rewritten review comment:
Refactoring Verified: Confirm Removal of
domainReferencesThe struct in
experiments/ClimaEarth/components/ocean/prescr_seaice.jlhas been simplified by removing the domain parameter (previously part of the type signature asD). This change is consistent with the intended refactoring. However, please be sure to manually verify that there are no leftover references to adomainfield anywhere in the codebase (for example, by running the providedrgcommand).
🏁 Script executed:
#!/bin/bash rg "domain:" -n --glob "*.jl"Length of output: 347
Refactoring Verification: Mixed
domainReferences DetectedIn
experiments/ClimaEarth/components/ocean/prescr_seaice.jlthe struct definition has been simplified by removing theD(domain) type parameter as expected. However, a subsequent search uncovered that some files still include references todomain::D:
test/flux_calculator_tests.jl:31experiments/ClimaEarth/components/atmosphere/climaatmos.jl:31Please review these instances to confirm whether they are meant to remain or if they require updating for consistency with the refactoring.
experiments/ClimaEarth/setup_run.jl (17)
38-38: New import ofCoupledSimulation.
Brings the newly refactored type into scope. No issues.
66-66: Addedstart_dateto function parameters.
Ensures the function can properly handle the new date-based logic. Looks fine.
86-93: Refactored docstring and convenience constructor.
- Docstring is updated to highlight the new usage of
CoupledSimulation(config_file)/CoupledSimulation(config_dict).- The actual function that takes
config_filenow delegates to theconfig_dictversion.Everything is coherent.
96-97:CoupledSimulation(config_dict::AbstractDict)definition.
Creating a dedicated function for direct dictionary input is a good modular approach.
115-115: Insertion ofstart_datein argument destructuring.
Part of the global rename fromdate0tostart_date. This matches the updated code style.
123-132: Additional parameters:diag_calendar_dt,evolving_ocean,use_coupler_diagnostics,output_dir_root.
These expansions align with the more flexible configuration approach. Watch for consistent usage in downstream logic.
302-302: Passingstart_datetoPrescribedOceanSimulation.
Ensures the ocean simulation can align with the simulation’s initial date. Correct usage.
516-518: Constructing aCoupledSimulationwithRef(start_date).
Storingstart_datein aRefcan help avoid copying in closure contexts. Valid design choice.
524-524:Ref(tspan[1])
Similarly placingtspan[1]into aRef. Maintains consistency for time references. No concerns.
599-600: Return statement forCoupledSimulation.
Neatly terminates the setup function. Looks good.
602-618: Refined docstring and early steps forrun!(cs::CoupledSimulation).
These lines detail the function’s purpose and precompilation steps. The guard for 2-step precompilation is a good performance measure.
630-635: While-loop replaced with acs.t[] <= cs.tspan[end]condition.
Clearly expresses the time-based iteration. Straightforward approach.
638-644: Logging simulation time metrics.
Sensible approach to measure performance. The SYPD logic is consistent with typical climate-model reporting.
649-650: Writing SYPD result tosypd.txt.
Straightforward I/O with correct usage ofopen. No issues.
654-654: Writingwalltime_per_atmos_step.
Same approach as above. No concerns.
658-658: Storing CPU memory usage.
Ensures resource usage is tracked. Fine approach.
664-668: Closing all diagnostics gracefully before returning.
Good housekeeping step. No issues.experiments/ClimaEarth/run_cloudy_slabplanet.jl (5)
175-176: Replacingdate0withstart_date.
Incorporating aDateTimeobject is consistent with the new approach.
279-279: Usingstart_datein the checkpoint schedule.
References the updated schedule approach. This is consistent with the new naming convention.
282-282: Usingstart_datein the albedo schedule.
Mirrors the checkpoint logic. Looks good.
367-367:cs.t[] = t
Direct assignment to track current time in the coupler. Straightforward approach.
371-371: InvokingTimeManager.maybe_trigger_callbackwithcs.
Uses the new function signature. Ensures albedo updates happen at the correct times.experiments/ClimaEarth/components/land/climaland_bucket.jl (3)
21-22: Documentation updated to reflect removed domain type parameterThe documentation has been correctly updated to reflect the simplified type signature of
BucketSimulationafter removing the domain type parameter.
31-39: Streamlined struct definition by removing domain dependencyThe
BucketSimulationstruct has been appropriately simplified by removing the domain type parameter and field, aligning with the PR's goal of making the simulation object more modular. This change is consistent with the broader restructuring that enables constructing a simulation without immediately executing it.
236-236: Constructor return simplified to match updated structThe constructor return statement now correctly omits the domain parameter, matching the simplified struct definition. This completes the domain removal refactoring, making the simulation object more focused on the model, integrator, and area fraction.
test/conservation_checker_tests.jl (4)
23-24: Cleaner field implementation for TestAtmosThe field implementations have been improved:
- Water field now uses the simpler
ones(s.i.space)instead of redundant multiplication- Energy field properly uses space-consistent type with
CC.Spaces.undertype(s.i.space)(1e6)These changes improve code clarity and ensure type consistency.
30-32: Improved TestOcean field implementationsThe TestOcean field implementations are now more efficient and type-safe:
- Water field uses explicit
zerosinstead ofones .* 0- Energy and area_fraction fields properly use the underlying space type with
CC.Spaces.undertypeThese changes ensure consistent numeric types and cleaner code.
38-40: Cleaned up TestLand field implementationsThe TestLand field implementations have been improved for clarity and type consistency:
- Energy and water fields now use explicit
zerosinstead ofones .* 0- Area fraction properly uses space-consistent type with
CC.Spaces.undertypeThese changes improve code clarity and numerical stability.
79-79: Time tracking added to CoupledSimulationA time reference parameter has been added to the
CoupledSimulationconstructor, enabling explicit time tracking. This is an essential change that supports the PR's goal of splittingsetup_and_runinto separate functions (setup,run!, andpostprocess), allowing users to construct a simulation without immediately executing it.Also applies to: 159-159
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
NEWS.md (1)
11-11: Minor grammar fix neededChange "was split in three functions" to "was split into three functions" for correct preposition usage.
-`setup_and_run` was split in three functions: +`setup_and_run` was split into three functions:🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: The preposition “into” seems more likely in this position than the preposition “in”.
Context: ...l/pull/1251)setup_and_runwas split in three functions: -CoupledSimulation,...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_INTO)
experiments/ClimaEarth/user_io/coupler_diagnostics.jl (1)
6-6: Update function signature documentationThe documentation should include the new
coupled_dtparameter that was added to the function signature.- coupler_diagnostics_setup(fields, output_dir, start_date, t_start, diag_calendar_dt) + coupler_diagnostics_setup(fields, output_dir, start_date, t_start, diag_calendar_dt, coupled_dt)experiments/ClimaEarth/setup_run.jl (1)
670-690: Improve documentation forpostprocessfunctionUnlike other functions,
postprocesslacks a documented signature and description. Consider adding a documentation string to maintain consistency with the other functions.+""" + postprocess(cs, conservation_softfail) + +Process the results after a simulation has completed, including generating +plots, checking conservation, and other diagnostics. +""" function postprocess(cs, conservation_softfail)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
NEWS.md(1 hunks)experiments/ClimaEarth/run_amip.jl(1 hunks)experiments/ClimaEarth/setup_run.jl(8 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(6 hunks)experiments/ClimaEarth/user_io/coupler_diagnostics.jl(2 hunks)experiments/ClimaEarth/user_io/postprocessing.jl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- experiments/ClimaEarth/user_io/postprocessing.jl
- experiments/ClimaEarth/user_io/arg_parsing.jl
🧰 Additional context used
🪛 LanguageTool
NEWS.md
[uncategorized] ~11-~11: The preposition “into” seems more likely in this position than the preposition “in”.
Context: ...l/pull/1251) setup_and_run was split in three functions: - CoupledSimulation,...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_INTO)
🪛 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 - 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: ci 1.10 - ubuntu-latest
🔇 Additional comments (11)
NEWS.md (1)
9-22: Good documentation for the new functionalityThe PR documentation clearly explains the split of
setup_and_runinto multiple functions and the renaming ofcalendar_dttodiag_calendar_dt, which will help users understand the changes.🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: The preposition “into” seems more likely in this position than the preposition “in”.
Context: ...l/pull/1251)setup_and_runwas split in three functions: -CoupledSimulation,...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_INTO)
🪛 markdownlint-cli2 (0.17.2)
9-9: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
experiments/ClimaEarth/user_io/coupler_diagnostics.jl (3)
14-14: Correctly updated function signatureFunction signature has been properly updated to reflect the parameter rename from
calendar_dttodiag_calendar_dtand to add the newcoupled_dtparameter.
17-17: Correctly updated variable usageVariable reference has been properly updated to use
diag_calendar_dtinstead ofcalendar_dt.
51-51: Properly added the new parameter to handler creationThe
dt = coupled_dtparameter is now correctly passed to theDiagnosticsHandlerconstructor.experiments/ClimaEarth/run_amip.jl (2)
46-52: Successfully refactored to use the new simulation execution flowThe code has been properly refactored to use the new execution flow, separating simulation setup, running, and postprocessing into distinct steps:
- Creating the simulation with
CoupledSimulation- Running the simulation with
run!- Postprocessing with
postprocessThis enhances clarity and modularity, allowing users more flexibility in how they execute simulations.
50-51: Consider addressing the TODO commentThere's a TODO comment about potentially removing the
conservation_softfailoption. It would be beneficial to create a follow-up issue to track this if it isn't addressed in this PR.Consider creating a GitHub issue to track the potential removal of the
conservation_softfailoption.experiments/ClimaEarth/setup_run.jl (5)
86-94: Well-documented constructor functionThe new
CoupledSimulationconstructor functions are well-documented, clearly explaining their purpose and functionality.
97-600: Successfully implementedCoupledSimulationconstructorThe implementation of the
CoupledSimulationconstructor effectively encapsulates all the setup logic that was previously insetup_and_run. The code is well-structured and maintains all the necessary initialization steps for the coupled simulation.
602-668: Well-implementedrun!function with timing and diagnosticsThe
run!function effectively handles:
- Optional precompilation for more accurate timing
- Execution of simulation steps within the time span
- Performance metrics (SYPD, walltime per step)
- Proper resource cleanup (diagnostic writers)
This separation of execution logic from setup improves code organization and maintainability.
692-710: Successfully updatedsetup_and_runfor backward compatibilityThe
setup_and_runfunction has been updated to use the new structure while maintaining backward compatibility, which is excellent for existing code that uses this function.
712-776: Well-implementedstep!functionThe
step!function successfully encapsulates all the logic for a single coupling step, allowing for more fine-grained control over simulation execution, which is particularly valuable for debugging.
Sbozzolo
force-pushed
the
gb/step
branch
2 times, most recently
from
f208714 to
f2e7019
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
NEWS.md (3)
9-10: Use setext heading for consistency with the rest of the document.
The code uses an atx heading (###) here, whereas markdownlint recommends setext style.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
16-17: Clarify usage context forstep!.
Consider adding a brief note about howstep!is typically called (e.g., debugging, partial progress). This can guide users reading release notes.
19-19: Consider markingsetup_and_runas deprecated for future releases.
If the new set of functions is preferred, you may want to officially deprecatesetup_and_runto direct users towardCoupledSimulation(...)+run!.experiments/ClimaEarth/user_io/postprocessing.jl (2)
15-15: Consider logging all extracted postprocessing variables for transparency.
Currently, onlyconservation_softfailis captured frompostprocessing_vars. If others are present, logging or verifying them could help diagnose missing parameters.
31-39: Use a more robust check for generating the leaderboard.
The logic currently hinges on the presence of "pfull" and a 3-month threshold. If other variables or conditions are needed in future, consider making them configurable or using a more flexible approach.experiments/ClimaEarth/setup_run.jl (5)
85-93: Docstring forCoupledSimulationconstructor.
This is a concise introduction. Consider adding a short example usage snippet to guide users.
495-502: Coupler diagnostics setup.
This block is straightforward. If these diagnostics can fail silently when misconfigured, consider implementing validation for required parameters.
615-618: Precompilation step.
Nice practice for performance. If precompilation overhead is significant, consider making it more configurable (e.g., user toggles).
630-669: Coupling loop logic.
Overall logic is clear. Consider adding partial-progress checkpoints or logging if the run is long.
722-723: Docstring forstep!.
Brief but effective. A usage example snippet would further illustrate typical calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
NEWS.md(1 hunks)experiments/ClimaEarth/run_amip.jl(1 hunks)experiments/ClimaEarth/setup_run.jl(8 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(6 hunks)experiments/ClimaEarth/user_io/coupler_diagnostics.jl(2 hunks)experiments/ClimaEarth/user_io/postprocessing.jl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- experiments/ClimaEarth/user_io/coupler_diagnostics.jl
- experiments/ClimaEarth/run_amip.jl
- experiments/ClimaEarth/user_io/arg_parsing.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 - 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: ci 1.10 - ubuntu-latest
🔇 Additional comments (9)
NEWS.md (2)
11-14: Good summary of the newly separated functions.
The bullet points read clearly, reflecting the new modular approach. No further issues observed.
21-23: Rename fromcalendar_dttodiag_calendar_dtis clear and consistent.
No issues found. The improved naming clarifies this parameter's usage.experiments/ClimaEarth/setup_run.jl (7)
38-38: New import forCoupledSimulation.
This addition clarifies the usage of the new constructor. No issues spotted.
123-132: Ensure clarity of newly added parameters.
Properties likediag_calendar_dtanduse_coupler_diagnosticsare newly exposed. Verify that default values or usage instructions are documented so users clearly understand them.
516-516: Initialization ofcs = CoupledSimulation{FT}.
The approach of parameterizing onFTfor floating types is consistent with broader Clima practices.
599-600: Return and finalize the constructor.
Everything appears cohesive. Just ensure that any post-initialization tasks are handled correctly before returningcs.
602-614: Docstring and signature forrun!.
This clarifies the new run workflow. Good usage of docstring headings.
671-699:postprocessfunction.
The layering of logic into a separate call withinrun_amip.jlor other scripts is a good approach for modularity. Looks good.
702-720:setup_and_runfunction expansions.
These changes unify the approach for config-based or direct dictionary-based setups, which is helpful. No major concerns.
| run!(cs) | ||
|
|
||
| # Postprocessing | ||
| # TODO: Remove this option? |
There was a problem hiding this comment.
We can remove it, but let's do it in a separate PR so we can get this one in today. conservation_softfail is used for 3 regular CI runs, and some longruns. The longruns are mostly soft fails anyway, and if any regular CI slabplanets fail conservation we can make them soft fail in the pipeline, which will increase visibility of which ones fail conservation rather than hiding it away with this argument.
Sbozzolo
force-pushed
the
gb/step
branch
2 times, most recently
from
7bc7649 to
596b2fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
NEWS.md (2)
9-10: Use setext headings to satisfy markdownlint.The static analysis tool warns that a setext heading style is expected here. Consider switching from the “atx” style (
###) to a setext heading (underlined with dashes) for consistency.-### Split `setup_and_run` in multiple functions. PR[#1251](...) +Split `setup_and_run` in multiple functions. PR[#1251](...) +----------------------------------------------------------🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
11-14: Clarify reference to four functions instead of three.Here, the text says "split into three functions" but then also mentions the newly introduced
step!. Consider updating it to avoid confusion (it's effectively four functions now).-`setup_and_run` was split into three functions: +`setup_and_run` was split into four functions:experiments/ClimaEarth/setup_run.jl (1)
85-94: Consider adding usage examples in the docstring.The docstring clearly describes purpose and arguments. Adding a small usage snippet can further clarify how to call these constructors.
""" CoupledSimulation(config_file) CoupledSimulation(config_dict) Set up a `CoupledSimulation` as prescribed by the given input. This struct is defined in the Interfacer module and contains all information about component models, diagnostics, timestepping, output directories, etc needed to run a coupled simulation. + + # Example Usage: + # cs = CoupledSimulation("path/to/config.yml") + # or + # cs = CoupledSimulation(config_dict) """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
NEWS.md(1 hunks)experiments/ClimaEarth/run_amip.jl(1 hunks)experiments/ClimaEarth/setup_run.jl(9 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(7 hunks)experiments/ClimaEarth/user_io/coupler_diagnostics.jl(2 hunks)experiments/ClimaEarth/user_io/postprocessing.jl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- experiments/ClimaEarth/run_amip.jl
- experiments/ClimaEarth/user_io/coupler_diagnostics.jl
- experiments/ClimaEarth/user_io/postprocessing.jl
- experiments/ClimaEarth/user_io/arg_parsing.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 - 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: ci 1.10 - ubuntu-latest
🔇 Additional comments (25)
NEWS.md (3)
16-18: No issues found.
19-19: No issues found.
21-23: Renaming reference looks good.experiments/ClimaEarth/setup_run.jl (22)
38-38: No issues found on the import line.
95-97: No issues found.
101-101: No issues found.
127-128: No issues found.
135-135: No issues found.
163-163: No issues found.
514-514: No issues found.
596-598: No issues found.
611-616: No issues found.
628-629: No issues found.
631-633: No issues found.
636-636: No issues found.
641-642: No issues found.
644-644: No issues found.
647-648: No issues found.
653-653: No issues found.
700-706: No issues found.
707-711: No issues found.
712-715: No issues found.
719-726: No issues found on the docstring block.
728-770: No issues found in the step! logic.
772-783: No issues found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
NEWS.md (1)
9-9: Update heading style to match document conventionThe heading style used here (ATX style with
###) doesn't match the setext style used for other headings in the document.-### Split `setup_and_run` in multiple functions. PR[#1251](https://github.com/CliMA/ClimaCoupler.jl/pull/1251) +Split `setup_and_run` in multiple functions. PR[#1251](https://github.com/CliMA/ClimaCoupler.jl/pull/1251) +-----------------------------------------------------🧰 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 (6)
NEWS.md(1 hunks)experiments/ClimaEarth/run_amip.jl(1 hunks)experiments/ClimaEarth/setup_run.jl(9 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(7 hunks)experiments/ClimaEarth/user_io/coupler_diagnostics.jl(2 hunks)experiments/ClimaEarth/user_io/postprocessing.jl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- experiments/ClimaEarth/user_io/coupler_diagnostics.jl
- experiments/ClimaEarth/run_amip.jl
- experiments/ClimaEarth/user_io/arg_parsing.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 - 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: ci 1.10 - ubuntu-latest
🔇 Additional comments (11)
NEWS.md (1)
11-22: Well-documented changes with clear explanationsThe documentation clearly explains the split of the monolithic function into four distinct components, which improves modularity and facilitates debugging. The renaming of
calendar_dttodiagnostics_dtalso enhances code clarity.experiments/ClimaEarth/user_io/postprocessing.jl (2)
15-15: Simplified parameter extractionParameter handling has been simplified to only extract what's needed.
31-41: Improved leaderboard plotting logicThe new implementation checks for the presence of required data rather than relying on configuration flags, which is more robust. The comment about pressure requirement is helpful as requested in previous reviews.
experiments/ClimaEarth/setup_run.jl (8)
85-94: Good documentation for CoupledSimulation constructorThe docstring clearly explains the purpose of this function and follows the previous reviewer's suggestion for more descriptive documentation.
95-98: Simple delegation pattern implemented correctlyThis version of the constructor takes a file path and delegates to the version that takes a dictionary, following best practices for constructor delegation.
101-598: Extracted logic from setup_and_run to CoupledSimulationThis refactoring cleanly separates initialization from execution, improving modularity. The implementation preserves all functionality while making the codebase more maintainable.
600-666: Well-structured run! function with performance trackingThe function includes precompilation, timing, appropriate error handling, and performance metrics. It also correctly manages resources by closing diagnostic writers.
638-643: Simulated years per day calculation fixedFollowing previous reviewer feedback, the SYPD calculation has been corrected to avoid dividing by walltime twice.
668-697: Clean postprocess function implementationThe function clearly separates postprocessing logic and handles conservation checks appropriately.
707-717: Simplified setup_and_run functionThe refactored function now delegates to the more modular components, making it easier to understand and maintain.
728-783: Well-structured step! function for simulation advancementThe implementation handles model updates, flux calculations, and communication barriers in a logical sequence. The code is well-commented and organized.
This commit introduces more structure in ClimaEarth. It splits `setup_and_run` in three functions: - `setup`, which produces a `CoupledSimulation` (hence, it was renamed to be `CoupledSimulation`, since it is just a constructor) - `run!`, which runs the simulation. This is supported by the introduction of `step!`, to take one single coupling step. This is particularly useful in debugging - `postprocess`, to make plots The advantage of this separation is that one can now construct a simulation without running it, and step a simulation one step at a time. Both of these are particularly useful in interactive work and debugging. This commit also renames `calendar_dt` to `diag_calendar_dt` to make it clearer that it refers to diagnostics.
This commit introduces more structure in ClimaEarth. It splits
setup_and_runin three functions:setup, which produces aCoupledSimulation(hence, it was renamed to beCoupledSimulation, since it is just a constructor)run!, which runs the simulation. This is supported by the introduction ofstep!, to take one single coupling step. This is particularly useful in debuggingpostprocess, to make plotsThe advantage of this separation is that one can now construct a simulation without running it, and step a simulation one step at a time. Both of these are particularly useful in interactive work and debugging.
This commit also renames
calendar_dttodiag_calendar_dtto make it clearer that it refers to diagnostics.Summary by CodeRabbit
New Features
Refactor
calendar_dttodiagnostics_dt.