Skip to content

Updates to improve input demo documentation#4035

Open
abbylewis wants to merge 4 commits into
PecanProject:developfrom
abbylewis:peprmt_vis
Open

Updates to improve input demo documentation#4035
abbylewis wants to merge 4 commits into
PecanProject:developfrom
abbylewis:peprmt_vis

Conversation

@abbylewis

Copy link
Copy Markdown
Collaborator

Description

Updated the PEPRMT demos, including:

  • More text explanation, especially in the first two demos
  • Added year as a PEcAn standard variable (was listed as a TODO)
  • New visualizations in the second demo so users see that it's working
    @infotroph not huge updates, but I think this should be helpful for CARB/etc to understand what's happening in the demos

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I agree that PEcAn Project may distribute my contribution under any or all of
    • the same license as the existing code,
    • and/or the BSD 3-clause license.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

│ ├── <runid>/ # Model-specific config copies (sometimes)
│ └── config.* # Generated model configs (e.g., SIPNET)
│ ├── <runid>/ # Model-specific configs, including run job and driver data
│ └── config.* # Generated model configs (not used for PEPRMT)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Is this true?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's definitely true PEPRMT doesn't use it and TBH I'm not sure what files this is talking about or what model uses them. Probably safe to delete this line.


Key subdirectories include `run/` and `out/` that contain files used to configure and run the model, files generated by the underlying ecosystem model, and PEcAn standard outputs used in downstream analyses. These are described in subsequent sections.

Additional outputs include logs, a `STATUS` file that records the steps of the workflow along with timestamps and whether each step was successful, and a copy of the `pecan.*.xml` file.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We might want to add something like this back but as is, I found it more confusing than helpful

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think these can be added to the file tree diagram -- I'll leave some suggestions to illustrate

@infotroph infotroph left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! I agree this is a big improvement. I left a few suggestions throughout, mostly with wording suggestions you can modify or reject freely.

Comment on lines 4 to 5
- "Aritra Dey"
- "David LeBauer"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At this point you've modified the source demo enough that it's probably appropriate to list yourself as the sole author and credit previous work with wording something like "This demo is patterned after PEcAn demo 1 (link here), which was written by Aritra Dey and David LeBauer". @AritraDey-Dev and @dlebauer does that make sense to y'all?

here::i_am("models/peprmt/demo_run/run_pecan_peprmt.qmd")
```

- A valid `pecan.xml` configuration file or use the example provided: `pecan/models/peprmt/demo_run/settings.xml`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I keep forgetting this is part of the "Before running this notebook...' list and reading it as a fragment. Would this rephrase help?

Suggested change
- A valid `pecan.xml` configuration file or use the example provided: `pecan/models/peprmt/demo_run/settings.xml`
- You will also need a valid `pecan.xml` configuration file, such as the example provided: `pecan/models/peprmt/demo_run/settings.xml`

│ ├── <runid>/ # Model-specific config copies (sometimes)
│ └── config.* # Generated model configs (e.g., SIPNET)
│ ├── <runid>/ # Model-specific configs, including run job and driver data
│ └── config.* # Generated model configs (not used for PEPRMT)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's definitely true PEPRMT doesn't use it and TBH I'm not sure what files this is talking about or what model uses them. Probably safe to delete this line.

```
```
demo_outdir/ # Root output directory
├── run/ # Configuration & execution metadata

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
├── run/ # Configuration & execution metadata
├── ensemble.samples.<ensemble_id>.Rdata # contains the selected parameter values for each ensemble member
├── run/ # Configuration & execution metadata

│ └── config.* # Generated model configs (not used for PEPRMT)
├── out/ # Raw model outputs by run ID
│ └── <runid>/ # E.g., daily PEPRMT output files
│ └── <runid>/ # E.g., PEPRMT output files

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
│ └── <runid>/ # E.g., PEPRMT output files
│ └── <runid>/ # E.g., PEPRMT output files
├── STATUS # timestamp and ok/error status of each workflow step
├── pecan.*.xml # copies of the run settings saved at different steps
├── run_manifest.csv # info about site, PFT, etc for each runid
└── samples.Rdata # draws from parameter distributions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rephrase all of the above freely, but key point is it seems worth documenting all these files here while we're explaining it.

```

model_output_marginal |>
mutate(site_id = sub("ENS-\\d+-", "", run_id)) |>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
mutate(site_id = sub("ENS-\\d+-", "", run_id)) |>
mutate(
site_id = sub("ENS-\\d+-", "", run_id),
CH4_flux = PEcAn.utils::ud_convert(CH4_flux, "kg/m2/s", "mg/m2/d")
) |>

select("site_id", "run_id", "posix", "CH4_flux") |>
ggplot() +
aes(x = posix,
y = CH4_flux * 1E6 * 60 * 60 * 24, # Convert from kg/m2/s to mg/m2/d

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
y = CH4_flux * 1E6 * 60 * 60 * 24, # Convert from kg/m2/s to mg/m2/d
y = CH4_flux,

In non-demo cases, you would of course create this file by actually running an MCMC sampler and would probably want to include all the parameters in the same sampling (and therefore only pass a single `trait.mcmc.RData` into the run).

Caution: If two or more PFTs define a parameter with the same name, `get.parameter.samples` will provide a draw from each of them and pass both as the `trait.values` argument to `write.config.PEPRMT`, which will then use whichever value appears first in `trait.values`, which is in turn determined by which PFT appears first in `settings$run$site$site.pft`.
TODO: PEcAn should probably have a cross-model policy on how to handle duplicate parameter definitions (but one that realizes there exist models that _do_ use both of them, e.g. models that explicitly represent more than one simultaneously grown PFT).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume you're following a style preference here, but to mildly defend my previous formatting: Since Markdown will render adjacent lines as one paragraph, when I'm writing something that's going to be version-controlled I like to start each sentence on a new line so that future edits can have smaller diff chunks. Not a big deal, but I find it does make a difference.

select("site_id", "run_id", "posix", "CH4_flux") |>
ggplot() +
aes(x = posix,
y = CH4_flux * 1E6 * 60 * 60 * 24, # Convert from kg/m2/s to mg/m2/d

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same suggestion to use ud_convert here as above

* `data/PEPRMT_specific_inputs/`
* `data/pfts/default_marginal/`
We also assume you have already run `02_input_demo.qmd`. This demo uses
these files created by in the second demp:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
these files created by in the second demp:
these files created by the second demo:

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.

2 participants