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

Document rose -N option in reST files #5056

Merged
merged 1 commit into from
Mar 31, 2021
Merged

Document rose -N option in reST files #5056

merged 1 commit into from
Mar 31, 2021

Conversation

maxrjones
Copy link
Member

Description of proposed changes

This PR adds the -N option to the reST documentation for rose.

Reminders

  • Make sure that your code follows our style. Use the other functions/files as a basis.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Describe changes to function behavior and arguments in a comment below the function declaration.
  • If adding new functionality, add a detailed description to the documentation and/or an example.

@maxrjones
Copy link
Member Author

@PaulWessel, the -N usage says that it's only valid for -Snradius, but there is no nradius modifiers listed for -S in the html or command-line docs. I can look into the code regarding nradius, or if you know off-hand what those modifiers do then I can add that to the docs.

@PaulWessel
Copy link
Member

Will look after a zoom that starts in 1 minute...

@maxrjones maxrjones mentioned this pull request Mar 31, 2021
5 tasks
@maxrjones maxrjones merged commit c1d1130 into master Mar 31, 2021
@maxrjones maxrjones deleted the docs-psrose branch March 31, 2021 23:26
@PaulWessel
Copy link
Member

@PaulWessel, the -N usage says that it's only valid for -Snradius, but there is no nradius modifiers listed for -S in the html or command-line docs. I can look into the code regarding nradius, or if you know off-hand what those modifiers do then I can add that to the docs.

The -S[n]radius was the old syntax, so this text will have to change. I see the -N requires -S basically, so silly to have two options needed to accomplish normalization of radii, then take square root of the normalized radii. I think this scheme would allow some simplification plus adding a -N option like histogram has:

  1. Let -N with no argument be a backwards compatible way to select sqrt(normalized-radii). This would be marked as a deprecated option.
  2. As an alternative to -N, introduce -S[+a] for what presently requires -S -N, with +a meaning "area normalization"
  3. Introduce -N0[+ppen] for drawing the best-fitting Least-squares von Mises pdf fit on top of the polar histogram (there are no robust fits yet for these distros, but if we add those later then best to have the same -N0|1|2 syntax as in histogram).

Thoughts?

@maxrjones
Copy link
Member Author

@PaulWessel, the -N usage says that it's only valid for -Snradius, but there is no nradius modifiers listed for -S in the html or command-line docs. I can look into the code regarding nradius, or if you know off-hand what those modifiers do then I can add that to the docs.

The -S[n]radius was the old syntax, so this text will have to change. I see the -N requires -S basically, so silly to have two options needed to accomplish normalization of radii, then take square root of the normalized radii. I think this scheme would allow some simplification plus adding a -N option like histogram has:

1. Let **-N** with no argument be a backwards compatible way to select sqrt(normalized-radii).  This would be marked as a deprecated option.

2. As an alternative to **-N**, introduce **-S**[**+a**] for what presently requires **-S -N,** with **+a** meaning "area normalization"

3. Introduce **-N0**[**+p**_pen_] for drawing the best-fitting Least-squares von Mises pdf fit on top of the polar histogram (there are no robust fits yet for these distros, but if we add those later then best to have the same **-N0**|**1**|**2** syntax as in histogram).

Thoughts?

Yes, I agree with all three points.

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.

2 participants