-
Notifications
You must be signed in to change notification settings - Fork 13
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
scale_x_logicle() not converging anymore #88
Comments
We'll investigate, in the meantime have a look here for a possible solution |
Hi Greg,
|
can't seem to reproduce it locally, my bioconductor docker image is 5-week old, let me pull the latest to try again |
Now I am on the latest bioc devel, can reproduce the same error
However, applying the same transformation to the same data outside of ggcyto, it succeeds
|
Thank you Mike, please let me know asap when you have found the cause of this error :-) |
I am having the same problem. Just stated appearing today. It was working perfectly in the morning but the issue appeared in the afternoon after I installed other packages (ggpubr, several dependencies were also updated/installed). Since, scale_x_logicle() is not critical, I tried excluding it. Although the error does not occur if scale_x_logicle() is removed, the resulting plots are empty with no data. Help!!! |
It seems to be introduced by the latest ggplot 3.4.0 release, not sure if it is a bug or breaking change from it. Still investigating... |
I downgraded and it worked (at least for the first few plots that I tried). Have a great weekend. |
Thanks for working on this amazing package |
@phauchamps flowjo_biexp should be equivalent to logicle and is more robust. widthBasis = -10^(2 * w)
maxRange=t
neg=a
pos=m In the latest patch, I just replaced |
Thank you Mike. However, are you sure of the parameters conversion from logicle to flojo_biexp(). I tried using the new github install of ggcyto, or by calling myself the scale_x_flowjo_biexp in my code using the same parameter conversion, and I get very different results than before. Actually, the new versions of the plots do not look very good. |
without the example code and data, we can tell where went wrong |
Hi Mike, I downgraded to ggplots2 3.3.6 in order to retrieve the previous scale_x_logicle() results.
When using ggcyto 1.26.0 (Bioc version), I get the first plot with scale_x_logicle(), and the second plot with scale_x_flowjo_biexp(). When using last ggcyto version from repo, I get twice the second plot. I think the parameters matching is not good. |
You are right, there seems to be no precise mapping between these two transformations, I am will rollback the previous commit. |
Thanks Mike. Do you think this patched version of ggplot2 will be released soon ? Because unfortunately my Bioc package submission is stuck to this issue right now. Since the convergence problem seems to appear whatever the to-be-displayed events are, my wild guess is that the new version of ggplot2 now generates a call to the logicle (or inverse logicle) function with some weird input values it did not do in the old version - maybe extreme values struck at the edges of the axes ? I would think that if you'd be able to trap which weird parameter values cause the logicle function call to crash, maybe you could 'neutralize' this call in your code ? Thanks again, Philippe |
Hi Mike, Digging more into this, I think I found the root cause, which is due to a new behaviour of ggplot2 since the new version 3.4.0. Here is a link to an issue I have just raised in ggplot2 repo : tidyverse/ggplot2#5105 It seems ggplot2 now calls the transformation function with c(-Inf, +Inf) as input (because it is the domain of definition that is set by default for the logicle transformation object). I don't know why this is now the case, since previous versions of ggplot did not do it. However, I think you can readily solve the problem by robustifying the C++ logicle() function call to extreme negative or positive input values (I guess returning NaN in these cases would do the trick, since the output of these calls does not seem to be used anywhere by ggplot2). Thanks, Philippe |
Hi, I'm the person who put in the PR that caused this issue. To explain briefly, ggplot2 now uses a transformation of the domain to cap the range that is fed into break calculations. Unfortunately, this appears to break some assumptions of the logicle transformation, which can't transform the [-Inf, Inf] domain that the transformation reports. I had a look at the code here: Lines 26 to 29 in ac3fcb1
And while it seems it would be best to fix this in the upstream package that defines Showing that I can reproduce the issue on my end: suppressPackageStartupMessages(
library(ggcyto)
)
data(GvHD)
fr <- GvHD[[1]]
p <- autoplot(fr, "FL1-H")
p + scale_x_logicle()
#> Error in logicle_transform(as.double(x), as.double(t), as.double(w), as.double(m), : Logicle Exception: T = 262144.000000, W = 0.500000, M = 4.500000, A = 0.000000
#> DidNotConverge: scale() didn't converge Next, if we override the domain with some large number that the transformation still accepts, we seem to get plot output. I'm not familiar enough with this type of plot to tell whether it is correct, but it seems alright to my untrained eyes. scale_x_logicle <- function(..., w = 0.5, t = 262144, m = 4.5, a = 0) {
myTrans <- logicle_trans(w = w, t= t, m = m, a = a)
myTrans$domain <- c(-1, 1) * 1e160
scale_x_continuous(..., trans = myTrans)
}
p + scale_x_logicle() Created on 2022-12-12 by the reprex package (v2.0.1) Best, |
@teunbrand I hesitate to change that default behavior of logicle transformation function, i.e. infinite values are not valid input. @phauchamps thank you so much spending time getting to the bottom of this issue. Let me know if the latest patch works for you |
Hi Mike, Latest patch works for me. You can now cherrypick the fix to the Bioc repo. Thanks to you and @teunbrand for solving this quickly! :-) Best, Philippe |
pushed |
Got if from BiocManager::install() in release version (3.16). |
develop branch is lagging behind, see the snapshot time here http://bioconductor.org/checkResults/3.17/bioc-LATEST/ |
Hi Mike and team,
I am currently in the process of submitting a new package to Bioconductor,
CytoPipeline
, which depends onggcyto::scale_x_logicle()
.Since the new Bioc 3.16 release, and in 3.17 devel, this function does not work anymore, regardless (it seems) of the inputs I provide to it. I constantly receive error messages like the following:
I can see that the same error occurs in
ggcyto
vignette building in the Bioc builds (see e.g. here )Have you already identified where this problem come from ? Any plan to have this solved soon ?
Thanks a lot,
Philippe
The text was updated successfully, but these errors were encountered: