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]: Updated ConfigWorkflow.rst to reflect changes to config_defaults.yaml (PI13) #1133

Merged
merged 10 commits into from
Sep 16, 2024

Conversation

jdkublnick
Copy link
Collaborator

DESCRIPTION OF CHANGES:
Updated ConfigWorkflow.rst to reflect recent changes to config_defaults.yaml in order to keep documentation up to date.
Type of change
[X ] This change requires a documentation update
TESTS CONDUCTED:
None required for documentation. The docs build successfully on my fork and can be viewed at: https://ufs-srweather-app-joshua-kublnick.readthedocs.io/en/text-us-336/BackgroundInfo/index.html
DEPENDENCIES:
N/A

DOCUMENTATION:
All documentation

ISSUE:
Issue #1132

CHECKLIST
[X ] My code follows the style guidelines in the Contributor's Guide
[X ] 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
[X ] 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).
[X ] My changes generate no new warnings
New and existing tests pass with my changes
[ X] Any dependent changes have been merged and published
LABELS (optional):
A Code Manager needs to add the following labels to this PR:

[X ] documentation

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.

@jdkublnick -

Thanks for going through the ConfigWorkflow.rst documentation and updating the entries that have been updated in config_defaults.yaml!

Cross referencing the changes that you have made in this PR with what is currently in the ConfigWorkflow.rst file at the HEAD of the develop branch, it looks like several of these entries are currently in the documentation. Looking at the commit history for your text/us-336 branch, it looks like the branch was created from a February 21, 2024 hash, with no other syncing with the develop branch after it was created.

Please merge the authoritative develop branch back into your text/us-336 branch. If you have any questions or require assistance, please let me know.

@gspetro-NOAA - While looking through the ConfigWorkflow.rst file and config_defaults.yaml, specifically in the FIX* sections, there is a disconnect occurring. In ConfigWorkflow.rst, the definition of variables is path to system directory, while in config_defaults.yaml, it is system directory where.

Should the definitions be updated in either the ConfigWorkflow.rst or config_defaults.yaml files (and which file should be updated, if you think it should), or do you feel that the differences for the FIX* variables are fine as is?

@gspetro-NOAA
Copy link
Collaborator

@jdkublnick I think the only changes required are from PR #1098. Pretty much everything else should remain as-is.

@gspetro-NOAA
Copy link
Collaborator

@MichaelLueken I think "Path to system directory..." is more accurate than "System directory where...". As far as I can tell, users would need to add the full path to the directory, not just the name of the directory, and the "Path to..." makes that clearer imo. Looking at, e.g., the Hera machine file, FIXaqm and similar variables are defined using a full path. I presume a user would need to change the path, not just the directory name, if their data is located somewhere else on their system. In that respect, it could be better to update the config_defaults.yaml file comments, but I have hesitated to overwrite what developers have put in. That said, the RST files are more clearly within our purview, and I have improved definitions where possible.

@MichaelLueken
Copy link
Collaborator

Thanks for the explanation, @gspetro-NOAA! I'm fine with keeping config_defaults.yaml as it currently is and providing more detailed information in the User Guide documentation.

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.

@jdkublnick

Thanks for merging the latest develop into text/us-336! The OMP_NUM_THREADS_RUN_FCST default value should be 2, rather than 1.

doc/UsersGuide/CustomizingTheWorkflow/ConfigWorkflow.rst Outdated Show resolved Hide resolved
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.

Thanks again, @jdkublnick!

The differences between the config_defaults.yaml and ConfigWorkflow.rst files appear to be addressed now.

Copy link
Collaborator

@gspetro-NOAA gspetro-NOAA left a comment

Choose a reason for hiding this comment

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

Requested changes were made. Looks good to me.

@MichaelLueken
Copy link
Collaborator

Moving forward with merging this work now.

@MichaelLueken MichaelLueken merged commit 2308fb5 into ufs-community:develop Sep 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[develop]: Updated ConfigWorkflow.rst to reflect changes to config_defaults.yaml (PI13)
3 participants