-
Notifications
You must be signed in to change notification settings - Fork 121
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 AQM to SRW user's guide #846
[develop] Add AQM to SRW user's guide #846
Conversation
@gspetro-NOAA, could you let me know how I can test this change in the doc? |
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.
@chan-hoo I was able to use readthedocs to build the updated User's Guide in your branch:
https://mikes-ufs-srweather-app-fork.readthedocs.io/en/feature-aqm_doc/AQM.html
In my review, I've noted dead links to a table that has been removed as part of this PR, minor spelling issues (code-clock rather than code-block keeping the build method from appearing in the documentation), and adding a ` to the end of the table and figure declarations.
Additionally, we have a new entry in the Code Contributor's Guide about not allowing binary files into be merged to the authoritative repository. You might want to reach out to @gspetro-NOAA for additional information on how to stage the file on the SRW App's wiki and then point to it in the AQM.rst file.
@MichaelLueken, thanks for the fix. I've updated it. @gspetro-NOAA, regarding @MichaelLueken's comment about a binary file, could you let me know how to resolve this issue? |
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.
@chan-hoo Table 7.2 still isn't appearing in the users guide. Adding a line between lines 238 and 239 allows the table to be generated and adds the table link on line 234.
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.
@chan-hoo One last update with respect to changing regional_workflow to workflow_tools, line 54 is still using regional_workflow, so it would be best to replace this as well.
Once this has been completed, I think that the only issue will be dealing with the binary file and ensuring that it shows up in the User's Guide.
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.
@chan-hoo There has been issues with the image appearing in rtd. I was able to correct this issue by aligning line 173 with line 172. Once this has been complete, I'll be ready to approve this PR. Thank you very much for working with @gspetro-NOAA and myself to get everything ready for this update to the documentation!
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.
@chan-hoo Thanks for making the requested modifications! Everything is now appearing correctly in the rtd. Approving now.
DESCRIPTION OF CHANGES:
Type of change
TESTS CONDUCTED:
ISSUE:
Fixes issue mentioned in #625
CHECKLIST
LABELS:
CONTRIBUTORS: