-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Check for constant ylim
susceptible to numerical precision issues
#368
Conversation
I've just noticed similar logic to deal with constant Hope that all seems sensible but I'm a little less clear on the logic in those latter two sections so please feel free to update as you see fit. Thanks |
Thanks for the report and patch! Do you have an example that triggers this issue? I'd like to add a unit test to make sure it doesn't reoccur. I applied your patch and I still get an error when I run this: plot(xts::xts(rep(10,10), Sys.Date()-10:1))
## Error in segments(xlim[1], y_grid_lines(get_ylim()[[2]]), xlim[2], y_grid_lines(get_ylim()[[2]]), :
## cannot mix zero-length and non-zero-length coordinates |
My use case is a little more complicated as I'm using both
Can I just check you've applied all the patches (my original plus the two follow ups)? I'll see if I can figure out how to squash them down into one to make it clearer EDIT: Just squashed and force pushed. Hope that's OK |
so replace strict `==` check, with `all.equal`, which includes a tolerance of `sqrt(.Machine$double.eps)`. wrap in a helper function `.perturbConstant` so can be reused in other spots where `yrange` is computed
Yep, that's fine! In the future, you don't need to do this unless I ask. I'm good with Git and can usually get what I need without users having to do extra work.
Here's the diff I have. Did you make change to any other file(s)? diff --git a/R/plot.R b/R/plot.R
index 05bc23d..dc77c0e 100644
--- a/R/plot.R
+++ b/R/plot.R
@@ -290,6 +290,18 @@ plot.xts <- function(x,
cs$Env$main <- main
cs$Env$ylab <- if (hasArg("ylab")) eval.parent(plot.call$ylab) else ""
+ # guard against constant yrange by (if necessary) perturbing values
+ .perturbConstant <- function(yrange) {
+ if(isTRUE(all.equal(yrange[1L], yrange[2L]))) {
+ if(yrange[1L] == 0) {
+ yrange <- yrange + c(-1, 1)
+ } else {
+ yrange <- c(0.8, 1.2) * yrange[1L]
+ }
+ }
+ return(yrange)
+ }
+
# chart_Series uses fixed=FALSE and add_* uses fixed=TRUE, not sure why or
# which is best.
if(is.null(ylim)){
@@ -305,14 +317,8 @@ plot.xts <- function(x,
# set the ylim based on all the data if this is not a multi.panel plot
yrange <- range(cs$Env$xdata[subset], na.rm=TRUE)
}
- if(yrange[1L] == yrange[2L]) {
- if(yrange[1L] == 0) {
- yrange <- yrange + c(-1, 1)
- } else {
- yrange <- c(0.8, 1.2) * yrange[1L]
- }
- }
- cs$set_ylim(list(structure(yrange, fixed=FALSE)))
+
+ cs$set_ylim(list(structure(.perturbConstant(yrange), fixed=FALSE)))
cs$Env$constant_ylim <- range(cs$Env$xdata[subset], na.rm=TRUE)
} else {
# use the ylim arg passed in
@@ -420,7 +426,7 @@ plot.xts <- function(x,
if(yaxis.same){
lenv$ylim <- cs$Env$constant_ylim
} else {
- lenv$ylim <- range(cs$Env$xdata[subset,1], na.rm=TRUE)
+ lenv$ylim <- .perturbConstant(range(cs$Env$xdata[subset,1], na.rm=TRUE))
}
exp <- quote(chart.lines(xdata,
@@ -452,9 +458,7 @@ plot.xts <- function(x,
if(yaxis.same){
lenv$ylim <- cs$Env$constant_ylim
} else {
- yrange <- range(cs$Env$xdata[subset,i], na.rm=TRUE)
- if(all(yrange == 0)) yrange <- yrange + c(-1,1)
- lenv$ylim <- yrange
+ lenv$ylim <- .perturbConstant(range(cs$Env$xdata[subset,i], na.rm=TRUE))
}
lenv$type <- cs$Env$type |
excluding them can leave us with an empty vector otherwise (when `ylim` is small)
So I think the source of the confusion is that I was working on the latest released version, (0.12.1), whereas I assume you were running on master. Once I checked out master, I once again encountered the error (seeing the same as you saw). After a decent amount of head scratching and trying to pick through the code (dusting it with |
Yes, I am working from the HEAD of master. Excellent sleuthing!
Thank you very much for continuing to dig into this! Your latest commit does prevent the error for my simple example. But the grid lines and series aren't plotted for some reason. I don't expect you to dig into that. Thanks again! |
Ah sorry, didn't notice that (was just relieved to finally see it now throw an error!) If if helps, I did notice that in one of the calls to |
We need this in several places, sometimes in an environment that doesn't have access to the functions defined in plot.xts().
I just pushed a commit that fixes the blank chart in my previous comment. Please give this a try and let me know if it's good or if there are issues. |
Sure thing, so I've just pulled your patch and had a look locally. Good news is that this certainly does look a lot better on the test series you provided suggested above. However, as I alluded to above, the data I was using that originally triggered this issue is a little more complicated. So I also tested a case closer to what I'm actually doing
(i.e., constant values, AND Not a dealbreaker as the patch you've got is definitely an improvement, but just thought I'd flag it. Thanks |
I appreciate you flagging that! I'll take another look. EDIT: The issue is that the main panel ylim are recalculated using the range of all columns. So that's why the max is 10. To fix it, we'd need a way to only use the range of all the series being plotted on a panel. |
The main panel ylim are recalculated using the range of all columns, even when multi.panel = TRUE and only one column is plotted on the panel. Only use the range of the series being plotted on the main panel. See joshuaulrich#368.
Thanks for the PR and all the help testing! I really appreciate it! |
Hi,
I'm attempting to use
plot.xts
to plot a series which contains constant values, however I encounter the errorAfter a little Googling, I was puzzled to see that this issue was fixed in #156
With the help of
debug
I found that in my case, numerical precision issues were causing my data to side step the check that had been implemented. The difference can only be seen withsprintf
. Debug console output:Proposal is to replace the strict
==
check, withall.equal
, which includes a tolerance ofsqrt(.Machine$double.eps)
Thanks
Please review the contributing guide before submitting your pull request. Please pay special attention to the pull request and commit message sections. Thanks for your contribution and interest in the project!