Skip to content

lim_args was updating xlim and ylim even if specified#221

Merged
vincentarelbundock merged 4 commits intograntmcdermott:mainfrom
mclements:main
Sep 23, 2024
Merged

lim_args was updating xlim and ylim even if specified#221
vincentarelbundock merged 4 commits intograntmcdermott:mainfrom
mclements:main

Conversation

@mclements
Copy link
Copy Markdown
Contributor

Nice package -- great to see base R graphics getting some attention.

This is a very minor bug fix. The following code shows that xlim and ylim were being ignored when ymin/etc are specified:

library(tinyplot)
x=y=0:1
conf.low=y-0.5
conf.high=y+0.5
par(mfrow=1:2)
plt(y~x,ymin=conf.low,ymax=conf.high,type="ribbon")
plt(y~x,ymin=conf.low,ymax=conf.high,type="ribbon",xlim=c(0,0.5),ylim=c(0,0.5))

The code changes lim_args() to check whether xlim or ylim are NULL.

@vincentarelbundock
Copy link
Copy Markdown
Collaborator

Thanks for taking a look at this!

Looks like it breaks some plots. In particular, it cuts the confidence intervals.

To run the test suite and check this yourself, you can comment out the "Linux" line in inst/tinysnapshot/helpers.R. Then,

pkgload::load_all()
tinytest::run_test_dir()

ribbon

pointrange_triangle

@mclements
Copy link
Copy Markdown
Contributor Author

Interesting - I want to cut the confidence intervals, particularly when the intervals are very wide. Semantically, I suggest that xlim and ylim should be followed when specified (as per base::plot()). Do you disagree?

@vincentarelbundock
Copy link
Copy Markdown
Collaborator

Yes, I agree that if the user explicitly sets xlim or ylim, the plot should set axes accordingly. I think your change is a necessary one.

However, I think that when ylim is not specified, the plot limits should be set to include the whole interval. This is not the case in your branch.

One of the examples I pasted above does not specify ylim, and yet cuts the axes:

https://github.com/grantmcdermott/tinyplot/blob/main/inst/tinytest/test-ribbon.R#L10

@mclements
Copy link
Copy Markdown
Contributor Author

I have further simplified the calculations for xlim and ylim.

I am getting errors for test-legend-gradient.R (6/7) and test-rect.R (1/2) due to different versions of libpng -- which may be due to how the test cases were generated.

Can you re-run the tests, please?

@vincentarelbundock vincentarelbundock merged commit 051fda9 into grantmcdermott:main Sep 23, 2024
@vincentarelbundock
Copy link
Copy Markdown
Collaborator

Thanks a lot for this @mclements ! It's a great and useful bug fix.

I updated a few snapshots (no visual difference at all) and everything passes. Merging now.

Cheers,

@grantmcdermott
Copy link
Copy Markdown
Owner

Thanks for the submission @mclements and for handling @vincentarelbundock!

I'll have a bit more breathing room towards the end of the week once I get some of these work deliverables over the line.

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.

3 participants