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 old diagnostics and ClimaAnalysis #2360

Merged
merged 6 commits into from
Jan 4, 2024
Merged

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Nov 13, 2023

Highlight of the changes:

  • Remove old diagnostics and post_processing pipelines
  • Use ClimaAnalysis to make plots for buildkite runs
  • Tar up artifacts so that it is easier to find the summary plots
  • Summary plots are now multi-page PDFs
  • Remove the diagnostics from the HDF5 restart files
  • Remove restart and rename functions (now that the diagnostics are removed from the HDF5 files, there is no longer any difference between a restart and a normal HDF5 file). This is a breaking change.

This pull request the post-processing pipelines are (for the most part) moved to using ClimaAnalysis. ClimaAnalysis takes care of all the file processing and reading and provides rudimentary plotting capabilities (when loaded with CairoMakie).

Going forward, there's going to be a unified way to produce plots for buildkite runs, with each job getting its post-processing function.

The entire post-processing pipeline has been condensed to a single file: ci_plots.jl. This file defines methods for the function

make_plot(::Val{jobid}, simulation_path)

where jobid is the id of the job for which the plots have to be produced.

For example, the function for the single column runs are:

ColumnPlots = Union{
    Val{:single_column_hydrostatic_balance_ft64},
    Val{:single_column_radiative_equilibrium_gray},
    Val{:single_column_radiative_equilibrium_clearsky},
    Val{:single_column_radiative_equilibrium_clearsky_prognostic_surface_temp},
    Val{:single_column_radiative_equilibrium_allsky_idealized_clouds},
}

function make_plots(::ColumnPlots, simulation_path)
    simdir = SimDir(simulation_path)
    short_names, reduction, period = ["ta", "wa"], "average", "1d"
    vars = [
        get(simdir; short_name, reduction, period) for short_name in short_names
    ]
    make_plots_generic(
        simulation_path,
        vars,
        time = LAST_SNAP,
        x = 0.0, # Our columns are still 3D objects...
        y = 0.0,
    )
end

Most post-processing steps will look like this. make_plots_generic is a new function defined in ci_plots that drives CairoMakie Figure/Axis creating and uses CairoMakieExt in ClimaAnalysis for heatmap or line plots (automatically selected depending on the dimensionality of the variable) and pages the output. The output is saved in temporary PDFs, which are all merged together to produce a multi-page PDF. To do so, I added Poppler_jll to the example environment.

Another change is with the buildkite artifacts. I collected all the files in two tar archives so that only up to three files are uploaded. This makes finding the summary.pdf easy and quick (otherwise one would have to hunt that file). To do so, I added Tar.jl as an explicit dependency to the example environment. Note that Tar.jl is required by Pkg.

@Sbozzolo Sbozzolo force-pushed the gb/remove_old_diagnostics branch 21 times, most recently from 83ca7b9 to c907b08 Compare November 15, 2023 18:33
@szy21 szy21 added this to the O3.2.2 Add a diagnostic module milestone Nov 16, 2023
@Sbozzolo Sbozzolo force-pushed the gb/remove_old_diagnostics branch 6 times, most recently from b292770 to 222cc10 Compare December 13, 2023 20:59
@Sbozzolo
Copy link
Member Author

Sbozzolo commented Jan 3, 2024

@charleskawczynski @szy21 @akshaysridhar I addressed the comments, feel free to have a look again when you have the chance.

Copy link
Member

@szy21 szy21 left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few more minor comments.

post_processing/ci_plots.jl Outdated Show resolved Hide resolved
post_processing/ci_plots.jl Outdated Show resolved Hide resolved
post_processing/ci_plots.jl Outdated Show resolved Hide resolved
post_processing/ci_plots.jl Outdated Show resolved Hide resolved
post_processing/ci_plots.jl Outdated Show resolved Hide resolved
post_processing/ci_plots.jl Outdated Show resolved Hide resolved
post_processing/ci_plots.jl Outdated Show resolved Hide resolved
@Sbozzolo Sbozzolo force-pushed the gb/remove_old_diagnostics branch 2 times, most recently from 356f20b to e6f18ec Compare January 3, 2024 23:26
@akshaysridhar
Copy link
Member

There seems to be a typo in MoisHeldSuarezPlots > MoistHeldSuarezPlots which is breaking one or more of the buildkite runs.

@Sbozzolo
Copy link
Member Author

Sbozzolo commented Jan 3, 2024

There seems to be a typo in MoisHeldSuarezPlots > MoistHeldSuarezPlots which is breaking one or more of the buildkite runs.

Thanks!

Now that the diagnostics are removed from the HDF5 file, there is no
longer any difference between `save_to_disk` and `save_restart`. The
functions are now consolidated into a single one that saves the state
without loss of information.
@Sbozzolo Sbozzolo added this pull request to the merge queue Jan 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 4, 2024
@Sbozzolo Sbozzolo added this pull request to the merge queue Jan 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 4, 2024
@Sbozzolo Sbozzolo added this pull request to the merge queue Jan 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 4, 2024
@Sbozzolo Sbozzolo added this pull request to the merge queue Jan 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 4, 2024
@Sbozzolo Sbozzolo added this pull request to the merge queue Jan 4, 2024
We don't need this, as we are already interpolating in the Remapper

We also need the newer version of ClimaCore because it contains the fix
for interpolation on face spaces
Without the diagnostics, save_to_disk and save_restart become identical.
This commit removes save_restart and renames save_to_disk to a more
descriptive and clear save_state_to_disk. This state is used for
restarts or to inspect the content of the state
@Sbozzolo Sbozzolo removed this pull request from the merge queue due to a manual request Jan 4, 2024
@Sbozzolo Sbozzolo added this pull request to the merge queue Jan 4, 2024
Merged via the queue into main with commit cdfa778 Jan 4, 2024
12 of 13 checks passed
@Sbozzolo Sbozzolo deleted the gb/remove_old_diagnostics branch January 4, 2024 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants