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

EAMxx: Initial Work on Developer Quick-start Guide #7043

Merged
merged 2 commits into from
Feb 26, 2025

Conversation

mjschmidt271
Copy link
Contributor

@mjschmidt271 mjschmidt271 commented Feb 20, 2025

Improve EAMxx documentation. Most notably, add a developer quick-start guide.

[BFB]


(Re-do of #7042 to switch branch to E3SM-Project rather than fork)

Putting this PR up as a draft to allow others to take a look and provide feedback.

Thus far, most of the work has been on the Developer Guide's Quick-start Guide. However, I've also made a mess of much of the rest of the pages with scatterbrained notes and partially-complete edits. So, consider it a ROUGH DRAFT at this point.

If we get to the point where The Dev QSG looks acceptable, I can submit a separate PR to get that merged quickly.

To Do

  • Find and Replace all scream $\to$ eamxx
    • NOTE: @jgfouca is working on a PR that de-screamifies all of EAMxx--wait until that merges.
  • Make sure any bells and whistles introduced to EAMxx docs play nice with the top-level E3SM mkdocs.yml

@AaronDonahue @bartgol @mahf708 @singhbalwinder @odiazib @jaelynlitz @overfelt

Copy link

github-actions bot commented Feb 20, 2025

PR Preview Action v1.6.0

🚀 View preview at
https://E3SM-Project.github.io/E3SM/pr-preview/pr-7043/

Built to branch gh-pages at 2025-02-26 00:08 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@bartgol bartgol force-pushed the mjs/eamxx/update-docs branch from da1c2cd to a5de4dc Compare February 20, 2025 22:05
@AaronDonahue AaronDonahue requested a review from mahf708 February 20, 2025 22:32
Copy link
Contributor

@mahf708 mahf708 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, we just need to fix the linter; @mjschmidt271 if you're done, I can push to this branch and fix everything; let me know

@bartgol bartgol added Documentation EAMxx PRs focused on capabilities for EAMxx BFB PR leaves answers BFB labels Feb 20, 2025
@mjschmidt271
Copy link
Contributor Author

@mahf708 I've got a few things I'd like to finish up, as well as carve out all the in-progress chaos outside of the dev quickstart.

Planning to have it ready to merge tomorrow (Friday), though, so I'll keep you posted!

@mahf708
Copy link
Contributor

mahf708 commented Feb 21, 2025

take your time, no rush on my side

@mjschmidt271
Copy link
Contributor Author

Alright... after a bare-knuckle brawl with the linter, I do believe this PR is all set.

A couple notes:

  • My main focus was on the Developer Quick-start Guide, Testing for Development, Code Structure, and partially on Automated Standalone Testing (test-all-eamxx).
  • There are pieces of the Developer Guide that still need some work.
  • Did some reorganizing and polishing outside of the Developer Guide directory, but should be mostly low-impact.
  • Changed the majority of SCREAM mentions to EAMxx.
    • Left several of them, though, since I wasn't sure if those paths would be changed yet.

Thanks!

@mahf708 @bartgol @AaronDonahue @jgfouca

@mjschmidt271
Copy link
Contributor Author

Hmm, looks like the gh-pages preview isn't completing due to an issue with the eamxx-params-docs-autogen script and related to the cime_config/eamxx_params.xml file.

I'm guessing it's related to the "scream" to "eamxx" repo-wide changes, but I can confirm the docs are building locally for me

@mahf708 mahf708 changed the title Improve EAMxx Documentation--Initial Work on Developer Quick-start Guide EAMxx: Initial Work on Developer Quick-start Guide Feb 23, 2025
Copy link
Contributor

@mahf708 mahf708 left a comment

Choose a reason for hiding this comment

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

@mjschmidt271 looks mostly good

  • you removed the common folder which was used to host the automatically generated params file, so I moved it to user (which will now trigger all testing since two edits were needed outside of components/eamxx/docs). I assumed you did this change intentionally, but if not, we can revert.
  • I encourage smaller updates in general. Big docs updates are usually harder to review meaningfully. I would focus on very specific item; for example, if you wanted to get test-all-eamxx docs done here, you could just focus on adding that one single file for that. Big changes make for confusing PRs and unlikely to get reviewed well enough, which usually means stuff gets lost and is slow to integrate.
  • I would avoid copying/pasting verbose tons of stuff that may change with no notice leaving docs stale. Examples include the printout from test-all-eamxx --help and the contents of a python file. I would just point the user to do these things on their own.

Ultimately, I think this is mostly fine and we can integrate it as-is, but let me know if you want to refocus it. We can always iterate on more fixes, etc., later. If you prefer to wait to integrate a cleaner version, then that's fine too.


If you are unsure of the cmake configuration for you development cycle, one
trick you can use is to run `test-all-eamxx` for the `dbg` test and just
copy the cmake command it prints (then ctrl-C the process).
Copy link
Contributor

Choose a reason for hiding this comment

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

we also have a --config-only option above (I don't think we should encourage users to do ctrl-C)

Copy link
Contributor Author

@mjschmidt271 mjschmidt271 Feb 23, 2025

Choose a reason for hiding this comment

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

Ah, thanks! Hadn't done a deep dive on this file yet, but I like the suggestion--changed!


#include "share/atm_process/atmosphere_process.hpp"

namespace eamxx {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are still in the namespace scream era

For the purposes of this guide, we will employ the working example
of adding a new [atmosphere processes](../processes.md)
to the MAM4xx aerosol library, specifically
the not-real, example process "**Ash-Injection**."
Copy link
Contributor

Choose a reason for hiding this comment

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

did you discuss this with luca/aaron?

Copy link
Contributor

Choose a reason for hiding this comment

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

because we have a whole exerise on this for the tutorial

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, yeah. I had a different stand-in process, but Luca recommended and wrote some of the Ash-Injection example. Simplified a little, as compared to the tutorial as well, I believe

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I basically half-copied the pompei example here. Later, we may consider moving this into its own page, and adding the polished pompei example from the tutorial. For now, I think it's good.

@@ -1,5 +1,5 @@
# Input-Output

In EAMxx, I/O is handled through the SCORPIO library, currently a submodule of
E3SM. The `scream_io` library within eamxx allows to interface the EAMxx
E3SM. The `eamxx_io` library within eamxx allows to interface the EAMxx
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have changed this one yet.

Copy link
Contributor

@bartgol bartgol left a comment

Choose a reason for hiding this comment

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

This is GREAT! I think it's a great step forward, so we should integrate sooner than later. I do have some comments we should address though.

For the purposes of this guide, we will employ the working example
of adding a new [atmosphere processes](../processes.md)
to the MAM4xx aerosol library, specifically
the not-real, example process "**Ash-Injection**."
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I basically half-copied the pompei example here. Later, we may consider moving this into its own page, and adding the polished pompei example from the tutorial. For now, I think it's good.

---

model_configuration.md cleaned up and linted

move, modify, clean up dev testing files

major updates for current PR finished
@mjschmidt271
Copy link
Contributor Author

rebasing/force-pushing...

Copy link
Contributor

@mahf708 mahf708 left a comment

Choose a reason for hiding this comment

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

🍏

bartgol added a commit that referenced this pull request Feb 26, 2025
Improve EAMxx documentation. Most notably, add a developer quick-start guide.

[BFB]
@bartgol bartgol merged commit 7bba735 into master Feb 26, 2025
20 of 21 checks passed
@bartgol bartgol deleted the mjs/eamxx/update-docs branch February 26, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB Documentation EAMxx PRs focused on capabilities for EAMxx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants