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

Add ClimateMachine example documentation #32

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Add ClimateMachine example documentation #32

merged 1 commit into from
Aug 4, 2021

Conversation

ilopezgp
Copy link
Contributor

No description provided.

@ilopezgp ilopezgp requested a review from odunbar June 10, 2021 17:50
@odunbar
Copy link
Collaborator

odunbar commented Jun 14, 2021

Hi Igacio, thanks for this! Comments as I see them:

  1. Title change(?): "HPC interfacing example: ClimateMachine" or maybe "ClimateMachine example - interfacing with HPC"
  2. Overview: Paragraph 1, maybe a slight restructure: "This examples uses EnsembleKalmanProcesses.jl to calibrate a climate model, showcasing a workflow which is compatible with HPC resources managed with the SLURM workload manager. The workflow is based on read-write input/output files, and as such it is capable of interfacing with dynamic models in different code languages, or with complicated processing stages. The dynamic model for this example is ClimateMachine.jl, an Earth system model currently under development at (CliMA)[hyperlink]."
  3. Overview Par 2: "results of the simulation" - do you mean "the chosen observed climate statistics from a simulation"
  4. Structure: 1. ekp_calibration.sbatch - also mention the "flow" is specifically a SLURM queue with job dependencies. You could also say this cycles 2. (3. 4.) (3. 4.) (3. 4.) ... for user specified number of iterations.
  5. Structure 2. The initial ensemble is stored in a set of parameter files
  6. Structure: 3. It submits a single... "Each copy of the script submits a single" ... given by a specific pair of parameters "read from a corresponding file"
  7. Structure: 4. updating the parameter ensemble, which are stored in a (the same/new) parameter files. Mention it reads data from a corresponding data file too. [we really want to emphasize the interface in this example]
  8. Structure: If you describe the loop properly above you can remove the "after the upate..." part
  9. Running the example: You don't need to talk about structure again
    8: Again we need to make sure we are clear about G(theta) just checking you refer to the processed data when you talk about this, - we do not store raw output?
  10. I assume you use EKI - mention that the solution is given by the final iteration. Maybe have a "Solution" section - as Melanie does.

In general - Could you add a section about the Bayesian problem. Just what the ingredients are for this - (1) what is the dynamic model (doesn't have to be too specific), but be very specific about (2) what the data we are using, and (3) what the priors are. Do you have a paper / references for this model too - which could be something easy to point people towards

Looking great thank you Ignacio!

@ilopezgp
Copy link
Contributor Author

Thanks for the input! I have followed most of your suggestions in this new version. Unfortunately, there is no good reference for the problem, but I have added some more information about the output, and the solution.

@odunbar
Copy link
Collaborator

odunbar commented Jun 17, 2021

A couple more comments:

  1. title: The title in the contents now doesn't match the title of the docs - maybe this needs to be changed in the Make.jl file?
  2. Intro: (my mistake I think) dynamic model should be dynamical model everywhere
  3. Structure: The 1->2->3->2->3->2 doesnt fit the enumerated numbering. I suggest separating ekp_calibration.sbatch script out of the enumeration, as this unique anyway, as the only one the user actually calls in practice; and then it runs the other scripts (which will be numbered 1,2,3).
  4. Running: You mention "no need to transform" In practice we must always define a transform, and here we use the identity transform, so could you say this instead
  5. Running: "For the construction of the forward model ... " reword [if this is correct] to "Overall the forward map G(\th) is given by applying an observation operator H to 0.5-hour batches of the dynamical model output \Psi. The action of H is average the horizontal velocity in time over the 30 min batches, therefore samples of data are time-averaged vertical profiles of this velocity.
    (PS for my own understanding - are you in a statistically stationary regime? if not how do you create the different realizations of your dynamical model w.r.t. initial conditions?)

Otherwise looks great thanks Ignacio!

@ilopezgp
Copy link
Contributor Author

ilopezgp commented Jul 6, 2021

Regarding the last point: In this simple example I am taking as data a single 30-min average horizontal velocity profile after initialization. Therefore, this is not in a statistically stationary regime. The problem is basically deterministic.

@odunbar odunbar requested a review from szy21 July 28, 2021 20:32
Copy link
Collaborator

@odunbar odunbar left a comment

Choose a reason for hiding this comment

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

LGTM - As Zhaoyi is a new user, I shall also let her add feedback if she sees possible improvements before merge

docs/src/examples/ClimateMachine_example.md Outdated Show resolved Hide resolved
@ilopezgp
Copy link
Contributor Author

ilopezgp commented Aug 4, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 4, 2021

Merge conflict.

@ilopezgp
Copy link
Contributor Author

ilopezgp commented Aug 4, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 4, 2021

Build succeeded:

@bors bors bot merged commit 535bf70 into main Aug 4, 2021
@bors bors bot deleted the il/cm_docs branch August 4, 2021 21:11
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.

3 participants