Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove Diagnostics module #953

Merged
merged 3 commits into from
Oct 16, 2024
Merged

remove Diagnostics module #953

merged 3 commits into from
Oct 16, 2024

Conversation

juliasloan25
Copy link
Member

@juliasloan25 juliasloan25 commented Sep 13, 2024

Purpose

Remove Diagnostics module in favor of ClimaDiagnostics and ClimaAnalysis

closes #954

will supersede #875 and #517

Content

This PR touches a lot of files, but most of it is just propagating a few main changes:

  • delete the ClimaCoupler Diagnostics module, tests, docs
  • use ClimaAtmos.Diagnostics to output atmos diagnostics, and ClimaDiagnostics to output coupler diagnostics
    • coupler diagnostics are defined in user_io/amip_diagnostics.jl
    • the diagnostics handler is added to our CoupledSimulation object, which results in changes in many files
  • add default AMIP diagnostics in driver
  • user_io/ci_plots.jl is generalized to meet the needs of our updated diagnostic plotting
  • artifacts are now output to a subdirectory of the run name, i.e. run_name/artifacts/, rather than an adjacent directory run_name_artifacts/
    • this isn't necessary for the other changes of the PR, but I found that it made more sense as I was looking at run outputs

To-do

  • generalize make_ci_plots to work for AMIP plots as well; update amip_paperplots function to use this
  • recreate AMIP paperplots using existing ClimaAtmos diagnostics defined in https://github.com/CliMA/ClimaAtmos.jl/blob/main/src/diagnostics/core_diagnostics.jl
    • coupler symbol -> atmos symbol:
    • ":T" -> "ta"
    • ":u" -> "ua"
    • ":q_tot" -> "hus"
    • ":q_liq_ice" -> "clw"
    • ":precipitation_rate" -> "pr"
    • ":T_sfc" -> "ts"
  • remove output_short_name, output_long_name
  • for the few quantities that aren't already computed in ClimaAtmos diagnostics, add new variables in climaatmos_extra_diags.jl
    • ":toa_fluxes": sum fluxes that can be accessed from atmos cache
    • ":turbulent_energy_fluxes": access calculated value from CoupledSimulation (requires extending ClimaDiagnostics functions)
  • remove amip_visualizer.jl
  • remove diagnostics from CoupledSimulation object
  • remove use_coupler_diagnostics flag
  • remove Diagnostics module and tests

Output checks

  • check slabplanet_atmos_diags now outputs toa_fluxes_net, previous coupler-added atmos diagnostics, and default diagnostics from atmos
  • check all amip runs output F_turb_energy
  • check plots are created for amip runs
    • atmos and coupled diagnostics

Previous output: AMIP paperplots

image

New output:

atmos_summary_2D.pdf

Screenshot 2024-10-14 at 3 22 15 PM

atmos_summary_3D.pdf (averaged over lon)

Screenshot 2024-10-14 at 3 23 43 PM Screenshot 2024-10-14 at 3 23 55 PM

coupler_summary_2D.pdf

Screenshot 2024-10-14 at 3 24 04 PM
  • I have read and checked the items on the review checklist.

@juliasloan25 juliasloan25 force-pushed the js/rm-diagnostics branch 4 times, most recently from eaf86d9 to b1ef4ee Compare September 17, 2024 18:51
callbacks,
dir_paths,
turbulent_fluxes,
thermo_params,
amip_diags_handler,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to include amip_diags_handler in the cs object so we have access to it inside the coupling loop without creating a closure

@juliasloan25 juliasloan25 force-pushed the js/rm-diagnostics branch 6 times, most recently from 444c29c to 8f8fd64 Compare October 4, 2024 16:25
@juliasloan25 juliasloan25 force-pushed the js/rm-diagnostics branch 5 times, most recently from 8d6ec54 to a34252a Compare October 14, 2024 21:42
@@ -4,9 +4,11 @@ import Statistics
import LinearAlgebra
import ClimaAtmos as CA
import ClimaAtmos: set_surface_albedo!
import ClimaAtmos.Parameters as CAP
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these previously weren't being correctly imported but it wasn't caught because we were including files that did import these

Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good to me! Maybe I'd just add a section in the docs to discuss how to add new diagnostics and give some links to the example you added and to the docs for ClimaDiagnostics and ClimaAnalysis

This generalizes the `make_ci_plots` function to `make_plots`
which takes in variable names as an argument instead of
hardcoding them within the function
The ClimaCoupler Diagnostics module had become redundant with
ClimaDiagnostics.jl, a package designed to provide robust
diagnostics across the CliMA ecosystem.
Here we remove ClimaCoupler.Diagnostics and instead use
ClimaDiagnostics.

We're able to retrive most of the diagnostics
we want directly from ClimaAtmos and ClimaLand, but also want
some that come from coupler-computed quantities, such as
`F_turb_energy`. In this PR we add this coupler quantity
to our output diagnostics using the ClimaDiagnostics interface.

This PR also removes the AMIP paperplots function, but this
functionality is replaced by the generalized `make_plots` function.
@juliasloan25 juliasloan25 merged commit 2601bdc into main Oct 16, 2024
6 checks passed
@juliasloan25 juliasloan25 deleted the js/rm-diagnostics branch October 16, 2024 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replace Diagnostics module with ClimaDiagnostics
2 participants