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 to a consistent definition of the r2 parameter for cones #3254

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

pshriwise
Copy link
Contributor

Description

Updating documentation for the cone r2 parameter for consistency after @NybergWISC pointed out some issues in one of the old doc strings that remained and noted possible confusion about units. Removing units (for now, depends on the discussion below) because the value is, as described in the doc strings, the square of the slope. If one's interpretation of the definition of the r2 parameter is that it's always relative to 1 cm from the apex, then I could see how this would have units one cm^2 also. Happy to go with a consensus on this.

@MicahGale you made some improvements here most recently. Can you take a look and provide some thoughts?

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable) N/A
  • I have followed the style guidelines for Python source files (if applicable) N/A
  • I have made corresponding changes to the documentation (if applicable) N/A
  • I have added tests that prove my fix is effective or that my feature works (if applicable) N/A

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

I'm good with the wording changes but the description should specify the units (e.g., "Parameter related to the aperture in [cm²]") which seem to have been removed.

@pshriwise
Copy link
Contributor Author

pshriwise commented Jan 8, 2025

I'm good with the wording changes but the description should specify the units (e.g., "Parameter related to the aperture in [cm²]") which seem to have been removed.

I think that's part of the motivation -- does this parameter truly have units?

Based on the way it's used in the quadric equations, it doesn't. Based on the definition in the docs, maybe.

At the end of the day, it's probably useful to have them there as it reinforces that this parameter is related to the value squared.

@NybergWISC
Copy link
Contributor

NybergWISC commented Jan 8, 2025

In my perspective, since the r2 parameter is essentially the square of the slope, it does not change as other length scales/dimensions change, so it should be labelled as dimensionless but I agree with Patrick that it depends on the written definition. Either the square of the slope (dr/d(X,Y,Z))^2 or what Patrick has written both describe the behavior of the parameter but will have different units. Based on how it is written now I am fine with the cm^2 units being included.

I believe my greatest concern came from the outdated documentation of similar classes like the Cone and OneSidedCone series which I think Patrick has fixed. Those definitions seemed to imply to me that the definition was (dr^2/d(X,Y,Z)) which didn't match up with the parameter behavior.

@paulromano
Copy link
Contributor

Oh yeah, you guys are right. That definitely does not have units. I like the description from Wolfram MathWorld: "ratio of radius to height at some distance from the vertex"

image

@MicahGale
Copy link
Contributor

I think it should be a unit-less quantity. I suggested a description that makes that clear I think but always give the easy way to calculate it.

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.

4 participants