Skip to content

Change in graphics::persp documentation example codes owing to a bug report #103

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

Closed
wants to merge 2 commits into from
Closed

Conversation

itsdebartha
Copy link

This is a change in documentation, owing to the bug report at https://bugs.r-project.org/show_bug.cgi?id=17699. Changes made:

  • modify the function f so that it does not return NAs
  • remove the scale of the function f
  • modify the expand parameter of persp to better represent the changes in the graph
  • modify the z-axis values of trans3d in both points and lines

This is a change in documentation, owing to the bug report at https://bugs.r-project.org/show_bug.cgi?id=17699.
Changes made:
- modify the function f so that it does not return NAs
- remove the scale of the function f
- modify the expand parameter of persp to better represent the changes in the graph
- modify the z-axis values of trans3d in both points and lines
@itsdebartha itsdebartha changed the title Change in documentation owing to a bug report Change in person documentation example codes owing to a bug report Jan 26, 2023
@itsdebartha itsdebartha changed the title Change in person documentation example codes owing to a bug report Change in persp documentation example codes owing to a bug report Jan 26, 2023
@itsdebartha itsdebartha changed the title Change in persp documentation example codes owing to a bug report Change in graphics::persp documentation example codes owing to a bug report Jan 26, 2023
@hturner
Copy link
Member

hturner commented Jan 30, 2023

Thinking about it some more, if we know x and y are not near zero - as we do when the sequence is even length - we can keep it simple and skip handling NAs. So I think you can just have f <- function(x, y) { r <- sqrt(x^2+y^2); sin(r)/r }.

Modifying the expand argument doesn't solve the issue of the z axis labels getting too close to the z axis title when the scaling is removed. I suggest instead adding cex.axis = 0.7, cex.lab = 0.7 to make all the axis text a little smaller in the second persp plot.

@itsdebartha
Copy link
Author

@hturner in that case, we have to check if the length of x is even or not and return f accordingly. Will it be better than this one as that will involve one more line of checking (i.e. checking if the length of x is even or not)?

@hturner
Copy link
Member

hturner commented Jan 30, 2023

I think as the function is just created for the purposes of this example, and we define x and y in the example, then we can ignore the fact that it is undefined for x = y = 0. That is, I don't think the function f() needs to handle this case, as it doesn't arise in the example.

@itsdebartha
Copy link
Author

That sounds great...
I'll do the changes

Updated changes made:
- removed handling of `NA` for more efficiency
- reverted the `expand` parameters back to 0.5 in both instances of the first example
- changed the axis and the label font size to make the graph better
@hturner
Copy link
Member

hturner commented Jan 31, 2023

Great. You can always make a comment when you submit to Bugzilla that handling NAs is unnecessary for this example, but if NAs are to be handled it would be neater to do this inside f() since that follows the definition of the sinc function. Then the R Core member that reviews the patch can decide which solution they prefer.

@itsdebartha
Copy link
Author

The commit 69abfba addresses this issue and solves the problem

@itsdebartha itsdebartha closed this Feb 6, 2023
@itsdebartha itsdebartha deleted the patch-1 branch February 6, 2023 05:50
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