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

Update catchability section with env Q #226

Merged
merged 3 commits into from
May 13, 2024
Merged

Conversation

e-perl-NOAA
Copy link
Collaborator

@e-perl-NOAA e-perl-NOAA commented May 6, 2024

See ss3-source-code PR #596 and issue #595.

@Rick-Methot-NOAA Can you make sure the extra parameters table example looks okay?
@iantaylor-NOAA Could you take a look just to make sure it all makes sense?

@e-perl-NOAA e-perl-NOAA added documentation Improvements or additions to documentation enhancement New feature or request Manual labels May 6, 2024
Copy link
Collaborator

@Rick-Methot-NOAA Rick-Methot-NOAA left a comment

Choose a reason for hiding this comment

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

Good news. I found the artifact all by myself.
One small change:
8.8.1 Mirrored Q with scale
better as:
8.8.1 Example parameter lines

@e-perl-NOAA
Copy link
Collaborator Author

@iantaylor-NOAA could you take a quick look at this just to make sure everything makes sense?

Copy link
Contributor

@iantaylor-NOAA iantaylor-NOAA left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you @e-perl-NOAA and @Rick-Methot-NOAA for cleaning up this section and documenting the new options. Sorry I was out sick so this response is slow.

I'm not sure why the github action to link the artifacts is being skipped, but tracking them down the old way shows me that there's a quirk in how the numbered lists work in HTML vs PDF. In the HTML case, it's a little strange to have the repeated numbers (as opposed to letters preceding the numbers in PDF). See screenshot below.

I also see a misspelled "requrired".

image

@Rick-Methot-NOAA
Copy link
Collaborator

Looks good regarding my requested change.
Acknowledging Ian's additional comments.

@e-perl-NOAA
Copy link
Collaborator Author

@iantaylor-NOAA they should be lettered now. I added another condition to the github action that adds the artifacts because I think it was only adding them on the initial PR, not on subsequent pushes but hopefully what I added will get it adding the artifacts on pushes.

@e-perl-NOAA e-perl-NOAA merged commit 7c4f208 into main May 13, 2024
4 checks passed
e-perl-NOAA added a commit that referenced this pull request May 23, 2024
* update catchability section about Q base, offset, and power
* update github action to add artifacts to PR to also add on push
@e-perl-NOAA e-perl-NOAA deleted the catchability-updates branch June 3, 2024 12:36
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 enhancement New feature or request Manual
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants