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

[develop]: Add Tutorial Chapter for the SRW App #556

Merged
merged 77 commits into from
Jan 31, 2023
Merged

[develop]: Add Tutorial Chapter for the SRW App #556

merged 77 commits into from
Jan 31, 2023

Conversation

gspetro-NOAA
Copy link
Collaborator

@gspetro-NOAA gspetro-NOAA commented Jan 23, 2023

DESCRIPTION OF CHANGES:

This PR adds a Tutorial chapter to the SRW App (and several plotting/png files, which makes this PR look much bigger than it is!). The Tutorial chapter outlines 5 severe weather cases. A complete tutorial is available for the first case. The hope is to fill out the others with each new SRW App release. To supplement the tutorial, a document on SSH access and data transfer was added to help users download plots from an HPC/cloud system to their local device. Additionally, a few Glossary updates were needed for the tutorial, as well as minor corrections to the plotting/TemplateVars explanations.

I am particularly interested in having someone with subject matter expertise review the section of the tutorial comparing plots. I think with some expert advice, the descriptions could be refined/improved. I'm hoping there aren't any errors, but it would be important to catch those, too!

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

TESTS CONDUCTED:

None required. I ran through my own instructions on Jet and NOAA Cloud (AWS). I also viewed the RTD version in my fork: https://srw-ug.readthedocs.io/en/text-tutorial/Tutorial.html

  • jet.intel
  • NOAA Cloud (AWS)

DEPENDENCIES:

PR #560
Some functionality is dependent on resolution of Issue #551 (addition of data to the standardized locations).

DOCUMENTATION:

All documentation.

ISSUE:

Resolves Issue #550.
Suggest closing Issue #551 before merging.

CHECKLIST

  • My code follows the style guidelines in the Contributor's Guide
  • I have performed a self-review of my own code using the Code Reviewer's Guide
  • I have commented my code, particularly in hard-to-understand areas N/A
  • My changes need updates to the documentation. I have made corresponding changes to the documentation
  • My changes do not require updates to the documentation (explain). N/A
  • My changes generate no new warnings
  • New and existing tests pass with my changes N/A
  • Any dependent changes have been merged and published --> Ideally need to resolve Issue Request for 2019061518 12-hour fcst HRRR/RAP Data on Level 1 Systems  #551

CONTRIBUTORS (optional):

@EdwardSnyder-NOAA

Copy link
Collaborator

@MichaelLueken MichaelLueken left a comment

Choose a reason for hiding this comment

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

@gspetro-NOAA Excellent work on the new tutorials! I'm excited to see the rest of the tutorial cases added to the new Tutorials chapter. I saw no issues with spelling, grammar, or the content of the documentation, so I will go ahead and approve these changes.

Copy link
Contributor

@dshawul dshawul 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 to me!

@MichaelLueken
Copy link
Collaborator

The necessary approvals for this PR have been given. I would like to give others the opportunity to review this work before it is merged. So, please look these changes over before the close of business today and I will merge this work tomorrow morning. Thanks!

task_run_post:
WTIME_RUN_POST: 00:20:00

Lastly, users must set the ``COMOUT_REF`` variable in the ``task_plot_allvars:`` section to create difference plots that compare output from the two experiments. ``COMOUT_REF`` is a template variable, so it references other workflow variables within it (see :numref:`Section %s <TemplateVars>` for details on template variables). ``COMOUT_REF`` should provide the path to the ``control`` experiment forecast output using single quotes as shown below:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I searched the current documentation for what COMOUT is short for or what meaning it's trying to convey ("common output directory"? If so, what's "common" about it?) but didn't see anything. In the past, I've tried to find this out from operations people at EMC but failed. Have you been able to find out? It would be great to have this (and other NCO variables) documented so users can form a better mental image of the NCO directory structure and the logic behind it. Just last week I was talking to a user and they told me it was very confusing -- not just this variable but other operational ones like NET -- because their nomenclature/purpose is not explained well (and he was an experienced modeler but not from the operations side).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, I haven't been able to find out... About the best I've got is a reference to the NCO Implementation Standards in the ConfigWorkflow chapter where we define all the relevant NCO-specific variables (https://srw-ug.readthedocs.io/en/text-tutorial/ConfigWorkflow.html#nco-specific-variables). I just added the same reference where I talk about COMOUT_REF in case users come across that first. I agree that it would be useful to know/explain more, but it seems like we've both hit a bit of a brick wall on this one... Part of me wonders if the issue is that the logic behind the naming is lost to the annals of history (i.e., operations folks just use it without necessarily knowing/wondering where the names came from originally). Will update if I find anything more though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gspetro-NOAA Yeah I think you're right about it being lost to the annals of history. In the plot_all_vars task, it seems like we can rename this variable to something more straightforward like POST_OUTPUT_DIR because as far as I can tell, this task isn't something that EMC/NCO runs and so doesn't require NCO-compliant names like COMOUT_.... Would that require a separate PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gsketefian I think it would be a separate PR, although I like the suggestion. That said, I believe the name was selected by @danielabdi-noaa, and he may have had reasons for selecting that name.

Copy link
Collaborator

@gsketefian gsketefian left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. All my remaining comments are just optional/editorials!

@MichaelLueken MichaelLueken linked an issue Jan 31, 2023 that may be closed by this pull request
@MichaelLueken
Copy link
Collaborator

@gspetro-NOAA Please let me know if you are okay with this PR as is, or if you would like some time to apply additional changes from @gsketefian before merging this work into develop. Thanks!

@gspetro-NOAA
Copy link
Collaborator Author

@MichaelLueken I think I'm good with the PR as is. I've addressed most of @gsketefian 's comments, and I think the one remaining would be best in a separate PR.

@MichaelLueken MichaelLueken merged commit 196ce68 into ufs-community:develop Jan 31, 2023
@mkavulich
Copy link
Collaborator

This commit has quadrupled the size of the repository (from 12MB to 56MB) with the addition of several large .gif files. While probably not enough of a concern to consider a history re-write, we should be more careful with what files we allow into the repository in the future.

@gspetro-NOAA
Copy link
Collaborator Author

This commit has quadrupled the size of the repository (from 12MB to 56MB) with the addition of several large .gif files. While probably not enough of a concern to consider a history re-write, we should be more careful with what files we allow into the repository in the future.

Oh dear! Didn't realize the .gifs would have such a dramatic effect. Did you run into a problem because of it? Regardless, I will see if I can reduce the size of the .gifs (e.g., by including fewer frames). Sorry!

@mkavulich
Copy link
Collaborator

Changing the files will only increase the size of the repository further due to how git history is stored. I think it's best to discuss the best way forward at the next Thursday meeting.

@mkavulich
Copy link
Collaborator

And for the record, no this hasn't caused any problems yet. Sorry if my wording was a bit terse: I don't expect everyone to know the nuances of git and we haven't explicitly covered this in our contributor's guide or other guidelines. This is a problem that many repositories have fallen into over the years, and I just wanted to bring it up as an issue ASAP so it can potentially be nipped in the bud.

mkavulich pushed a commit that referenced this pull request Feb 2, 2023
This PR adds a Tutorial chapter to the SRW App (and several plotting/png files, which makes this PR look much bigger than it is!). The Tutorial chapter outlines 5 severe weather cases. A complete tutorial is available for the first case. The hope is to fill out the others with each new SRW App release. To supplement the tutorial, a document on SSH access and data transfer was added to help users download plots from an HPC/cloud system to their local device. Additionally, a few Glossary updates were needed for the tutorial, as well as minor corrections to the plotting/TemplateVars explanations.

2023-02-02 NOTE:
This commit has been re-written to remove binary image files from the repository.

---------

Co-authored-by: gspetro <[email protected]>
mkavulich pushed a commit to mkavulich/ufs-srweather-app that referenced this pull request Feb 6, 2023
This PR adds a Tutorial chapter to the SRW App (and several plotting/png files, which makes this PR look much bigger than it is!). The Tutorial chapter outlines 5 severe weather cases. A complete tutorial is available for the first case. The hope is to fill out the others with each new SRW App release. To supplement the tutorial, a document on SSH access and data transfer was added to help users download plots from an HPC/cloud system to their local device. Additionally, a few Glossary updates were needed for the tutorial, as well as minor corrections to the plotting/TemplateVars explanations.

---------

Co-authored-by: gspetro <[email protected]>
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.

Add a Tutorial Chapter to the SRW App Documentation
7 participants