-
Notifications
You must be signed in to change notification settings - Fork 273
Demo 2 Quarto notebook tutorial for PEcAn uncertainty analysis #3570
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
Demo 2 Quarto notebook tutorial for PEcAn uncertainty analysis #3570
Conversation
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
7033438 to
122ef77
Compare
dlebauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR, it is off to a great start and it is clear that you have a good understanding of how these analyses work.
I've started reviewing the notebook and have a few suggestions for how and what information is presented - I think it will be most effective if I flesh out some of the more technical details, and determine what information can be provided as cross references to publications vs what should be explained here.
But I'd also like to request some changes from the workflow side. Many of these are changes to Demo 1 that I suggested (AritraDey-Dev#6), and I think these should be reflected in this repository.
Some specific changes that come to mind:
- eliminate hard coded paths like /pecan and /data
- use R's
file.copy()function in place ofsystem('cp ...) - don't assume user will be running in Docker with local binaries and absolute paths pre-installed
- the only difference between the pecan.xml files should be related to uncertainty / ensemble / sensitivity analysis. File paths and other settings should be the same. This helps clarify and identify the meaningful changes that are necessary.
- Make directory naming consistent - not clear why there is 01_Demo_basic_run, 02_Demo_Uncertainty_Analysis, and Demo_02_Uncertainty_Analysis.
These changes should help keep the tutorial easier to understand and maintain. Let me know if you’d like help implementing them—happy to pair on a walkthrough or review the follow-up PR. Thanks!
|
Yes, once the Demo 1 PR gets merged, I’ll add the changes you mentioned above here. |
12f7e60 to
b9cb3bf
Compare
documentation/tutorials/Demo_02_Uncertainty_Analysis/uncertainty.qmd
Outdated
Show resolved
Hide resolved
|
Great work getting Demo 1 in #3531 merged 🎉 — that’s a big milestone! Now that that one is merged, there is still a fair bit of cleanup to do here, and it will require updating Demo 1 as well. It’s all very doable, and I think the payoff will be a much more polished and user-friendly Demo 2. Here are some steps:
This is very close! While this is still a bit of work, it’s mostly structural and editorial / pedagogical. Once you have successfully merged the branches, consolidated the background, and made sure that all information from the original Demo 2 is in the new version, I can take a final pass at the writing. |
documentation/tutorials/Demo_02_Uncertainty_Analysis/uncertainty.qmd
Outdated
Show resolved
Hide resolved
8d965d1 to
cfface8
Compare
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
| legend("topright", legend = c("LAI", "Above Ground Wood"), col = c("darkgreen", "brown"), lty = 1) | ||
| ``` | ||
|
|
||
| # 18. PEcAn Outputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is almost same as section 9 which tells about pecan outputs,also in this part as it mentions trait and meta analysis which we are not doing in this notebook,so can we remove this part @dlebauer ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this part and push the changes.
I've created #3661 to track future re-implementation of a meta-analysis tutorial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And have implemented the requested revisions in AritraDey-Dev#10
|
@AritraDey-Dev @dlebauer anything left to do or is this ready to merge? |
|
Most of the core work for this PR is done; just waiting for @dlebauer’s review to get it merged. |
….qmd demo - Enable auto-numbered and cross-referenced sections
dlebauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've consolidated sections 9&18, move some content to Demo 1, and automated section numbering 🤞🏻.
Please review AritraDey-Dev#10 and make or request any additional changes. Once that is complete, this should be ready to merge!
|
|
||
| Editing the more interesting settings to change the PFT (`settings$pfts`) or extend the run (`settings$run$end.date`) is an optional exercise for the user. For example, you could change the pft or the end date, but this requires additional parameter files and meteorological inputs. This includes files containing parameters for that PFT (`settings$pfts$pft$posterior.files`), or a climate file (`settings$run$met$path$path1`) that extends to the desired simulation period. | ||
|
|
||
| # 7. Create Output Directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AritraDey-Dev I think that this is automated by the prepare.settings() function, so the section can be removed. Please confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes indeed this is not needed.
| legend("topright", legend = c("LAI", "Above Ground Wood"), col = c("darkgreen", "brown"), lty = 1) | ||
| ``` | ||
|
|
||
| # 18. PEcAn Outputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this part and push the changes.
I've created #3661 to track future re-implementation of a meta-analysis tutorial.
| legend("topright", legend = c("LAI", "Above Ground Wood"), col = c("darkgreen", "brown"), lty = 1) | ||
| ``` | ||
|
|
||
| # 18. PEcAn Outputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And have implemented the requested revisions in AritraDey-Dev#10
documentation/tutorials/Demo_02_Uncertainty_Analysis/uncertainty.qmd
Outdated
Show resolved
Hide resolved
documentation/tutorials/Demo_02_Uncertainty_Analysis/uncertainty.qmd
Outdated
Show resolved
Hide resolved
documentation/tutorials/Demo_02_Uncertainty_Analysis/uncertainty.qmd
Outdated
Show resolved
Hide resolved
|
|
||
| 1. Disabling database write operations because we are not using a database | ||
| ```{r disable-db-write} | ||
| settings$database <- NULL # Disable database operations for this demo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed for demo 1, I don't see why this is needed -- setting a list entry to NULL is the same as removing it, so why not just delete it from the XML in the first place?) -- and I'm really quite sure the user should not need to do it. If deleting this line makes the demo work wrong, please file an issue with a reprex so that we can fix whatever makes this distracting hack necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for now deleting this line will not make the demo work. I agree that this line is unrelated to the demo. This is probably an issue with the PEcAn DB package.
documentation/tutorials/Demo_02_Uncertainty_Analysis/uncertainty.qmd
Outdated
Show resolved
Hide resolved
| ├── run/ # Configuration & execution metadata | ||
| │ ├── runs.txt # List of run IDs (one per model realization) | ||
| │ ├── <runid>/ # Model-specific config copies (sometimes) | ||
| │ └── config.* # Generated model configs (e.g., SIPNET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have never seen config.* in the rundir! What module generates them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,A lot of refactoring need to be done in this part.i will fix this.
documentation/tutorials/Demo_02_Uncertainty_Analysis/uncertainty.qmd
Outdated
Show resolved
Hide resolved
| ├── sensitivity.analysis/ # Sensitivity raw outputs & figures | ||
| │ ├── sensitivity.output.*.Rdata | ||
| │ └── sensitivity.analysis.*.pdf | ||
| ├── ensemble.analysis/ # Ensemble summaries & figures | ||
| │ ├── ensemble.Rdata | ||
| │ ├── ensemble.analysis.*.pdf | ||
| │ └── ensemble.ts.*.pdf | ||
| ├── variance.decomposition/ # Created when variance decomposition runs | ||
| │ └── variance.decomposition.*.pdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm used to seeing all of these saved directly in outdir without their own subdirectories -- what process creates these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 97c15b9
Minor changes to Demo 2
Signed-off-by: Aritra Dey <[email protected]>
Co-authored-by: Chris Black <[email protected]>
…ty.qmd Co-authored-by: Chris Black <[email protected]>
…ty.qmd Co-authored-by: Chris Black <[email protected]>
…ty.qmd Co-authored-by: Chris Black <[email protected]>
…ty.qmd Co-authored-by: Chris Black <[email protected]>
…ty.qmd Co-authored-by: Chris Black <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Co-authored-by: Chris Black <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
Signed-off-by: Aritra Dey <[email protected]>
dlebauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for your persistence addressing all of the feedback!
579b9d9
Description
Added a Quarto notebook tutorial for PEcAn uncertainty analysis (uncertainty.qmd) that demonstrates ensemble and sensitivity analysis workflows. The tutorial includes:
Motivation and Context
This change introduces Demo2 for the Quarto notebook focused on uncertainty analysis. It serves as a continuation and enhancement of the work initiated in Demo1. The goal is to extend the functionality and provide a more comprehensive demonstration of uncertainty analysis within the PEcAn framework.
Review Time Estimate
Types of changes
Checklist: