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

Interface Postprocessor v2 #225

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Interface Postprocessor v2 #225

merged 1 commit into from
Jan 18, 2023

Conversation

LenkaNovak
Copy link
Collaborator

@LenkaNovak LenkaNovak commented Jan 13, 2023

Purpose

Modularize the offline postprocessor utility. 3826273012

To-do


  • I have read and checked the items on the review checklist.

This was referenced Jan 13, 2023
@LenkaNovak LenkaNovak marked this pull request as ready for review January 14, 2023 00:02
Copy link
Member

@juliasloan25 juliasloan25 left a comment

Choose a reason for hiding this comment

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

looks good! just some small comments

@@ -0,0 +1,24 @@
# PostProcessor

This module contains functions for postprocessing model data that was saved during the simulation by `ClimaCoupler.Diagnostics`). This module is used for offline regridding, slicing and spatial averages. It can also handle data from
Copy link
Member

Choose a reason for hiding this comment

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

line length too long, extra ")" after ClimaCoupler.Diagnostics

@@ -0,0 +1,64 @@
include("plot_helper.jl")
Copy link
Member

Choose a reason for hiding this comment

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

could make plot_helper a module and import it instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about this too, but overall I concluded that this belongs to the code that is too specific to the experiment (along with specific diagnostics definitions), so it should really live in the experiments folder. If the plot helper is something we'll end up using in the long term for multiple experiments, then we can modularize it then. :)

return all_plots
end

using Downloads
Copy link
Member

Choose a reason for hiding this comment

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

move imports to top of file?

of the last monthly mean file. Any specific plot customization should be done here.
"""
function amip_paperplots(
post_spec,
Copy link
Member

Choose a reason for hiding this comment

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

could specify argument types

of a particular monthly mean dataset (specified by `month_date`). Any plot NCEP- specific
customization should be done here.
"""
function ncep_paperplots(
Copy link
Member

Choose a reason for hiding this comment

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

could specify argument types

end

"""
download_read_nc(https, tmp_dir, ncep_vname )
Copy link
Member

Choose a reason for hiding this comment

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

comment doesn't match function signature

using ClimaCoupler.PostProcessor: PostProcessedData, ZLatData, LatLonData, DataPackage, ZLatLonData
using Plots

function plot(post_data::DataPackage; zmd_params = (;), hsd_params = (;))
Copy link
Member

Choose a reason for hiding this comment

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

could add docstrings

@@ -14,7 +14,7 @@ export create_space, gen_ncdata

"""
create_space(FT; comms_ctx = ClimaComms.SingletonCommsContext(),
R = FT(6371e3), ne = 4, polynomial_degree = 3)
R = FT(6371e3), ne = 4, polynomial_degree = 3, , nz = 1)
Copy link
Member

Choose a reason for hiding this comment

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

extra comma before nz

tests

cleanup

test fix

check in test Manifest

check in test Manifest

temp

temp

dep

temp

clean

temp fix

dep fix

dep fix

dep fix

test

try

format

rebase fixes

mpi test rebase

clean

amip data name fix

postprocessor mod v2

format+deps

fix

fix

dep fix

ncep plot fix

revs

rev fix
@LenkaNovak LenkaNovak mentioned this pull request Jan 17, 2023
2 tasks
@LenkaNovak
Copy link
Collaborator Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 18, 2023

@bors bors bot merged commit 50c5628 into main Jan 18, 2023
@bors bors bot deleted the ln/interface-postprocessor2 branch January 18, 2023 03:28
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.

None yet

2 participants