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

scale_x_logicle() not converging anymore #88

Open
phauchamps opened this issue Nov 25, 2022 · 22 comments
Open

scale_x_logicle() not converging anymore #88

phauchamps opened this issue Nov 25, 2022 · 22 comments

Comments

@phauchamps
Copy link

Hi Mike and team,

I am currently in the process of submitting a new package to Bioconductor, CytoPipeline, which depends on ggcyto::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:

Error: Logicle Exception: T = 262144.000000, W = 2.000000, M = 6.420000, A = 0.000000
   DidNotConverge: scale() didn't converge

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

@gfinak
Copy link
Member

gfinak commented Nov 25, 2022

We'll investigate, in the meantime have a look here for a possible solution
RGLab/flowCore#41

@phauchamps
Copy link
Author

Hi Greg,
Yes I knew already about the post you are pointing to.
However, what we have here is a rather serious issue with the logicle function from flowCore, since even default parameters are generating this convergence error, and the ggcyto vignette does not work anymore.

scale_x_flowJo_biexp(), on the other hand, while not generating errors, is no option for me, as it does not look the same, and does not correspond to the parameters obtained using flowCore::estimateLogicle() - unless you know of an exact one to one correspondance between the flowjo_biexp() parameters and the logicle() parameters: I tried myself, for the flowjo_biexp parameters :

maxValue = logicleParams$t,
widthBasis = -10^logicleParams$w,
pos = logicleParams$m - logicleParams$w,
neg = logicleParams$a

@mikejiang
Copy link
Member

can't seem to reproduce it locally, my bioconductor docker image is 5-week old, let me pull the latest to try again

@mikejiang
Copy link
Member

Now I am on the latest bioc devel, can reproduce the same error

> library(ggcyto)
> data(GvHD)
> fr <- GvHD[[1]]
> p <- autoplot(fr, "FL1-H")
> p + scale_x_logicle() #flowCore logicle scale
Error: Logicle Exception: T = 262144.000000, W = 0.500000, M = 4.500000, A = 0.000000
DidNotConverge: scale() didn't converge

However, applying the same transformation to the same data outside of ggcyto, it succeeds

> lgcl <- logicleTransform()
> trans <- transformList(c("FL1-H"), lgcl)
> after <- transform(fr, trans)
> range(fr)["FL1-H"]
    FL1-H
min     1
max 10000
> range(after)["FL1-H"]
        FL1-H
min 0.5050419
max 3.0772470

@phauchamps
Copy link
Author

Thank you Mike, please let me know asap when you have found the cause of this error :-)

@flbgmazo
Copy link

flbgmazo commented Dec 2, 2022

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!!!

@mikejiang
Copy link
Member

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...
In the meantime, you can downgrade ggplot to 3.3.x which should resolve your issue for now

@flbgmazo
Copy link

flbgmazo commented Dec 2, 2022

I downgraded and it worked (at least for the first few plots that I tried). Have a great weekend.

@flbgmazo
Copy link

flbgmazo commented Dec 2, 2022

Thanks for working on this amazing package

@mikejiang
Copy link
Member

@phauchamps flowjo_biexp should be equivalent to logicle and is more robust.
The correct mapping between two is

widthBasis = -10^(2 * w)
maxRange=t
neg=a
pos=m

In the latest patch, I just replaced logicle_trans with flowjo_biexp_trans in scale_x_logicle() function so that it is free from DidNotConverge exception.
Here is the plot now using biexp_trans under the hood
image

@phauchamps
Copy link
Author

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.

ggplotevents-1d-logicle-single new

@mikejiang
Copy link
Member

without the example code and data, we can tell where went wrong

@phauchamps
Copy link
Author

Hi Mike, I downgraded to ggplots2 3.3.6 in order to retrieve the previous scale_x_logicle() results.
Here is a minimal reproduceable example, using fcs file in .zip in attachment


library(ggcyto)

# TO UPDATE
datasetPath <- "/Datasets"
fileName <- "sample_Donor1.fcs"

mySample <- read.FCS(filename = paste0(datasetPath, "/", fileName),
                     transformation = "linearize",
                     alter.names = FALSE,
                     name = "OMIP-21",
                     truncate_max_range = TRUE,
                     min.limit = NULL)



a=1
w=2
[IssueLogicle_20221205.zip](https://github.com/RGLab/ggcyto/files/10152759/IssueLogicle_20221205.zip)
[IssueLogicle_20221205.zip](https://github.com/RGLab/ggcyto/files/10152763/IssueLogicle_20221205.zip)

m=7
t=262144

xChannel <- "450/50Violet-A"

p <- ggplot(
    data = mySample,
    mapping = aes(x = .data[[xChannel]])
) +
    geom_density(fill = "lightblue", alpha = 0.2, na.rm = TRUE) +
    xlab(xChannel)

p + scale_x_logicle(a = a,
                    w = w,
                    m = m,
                    t = t) +
    ggtitle("scale_x_logicle")
                         

p + scale_x_flowjo_biexp(maxValue = t,
                         widthBasis = -10^(2*w),
                         pos = m,
                         neg = a) +
    ggtitle("scale_x_flowjo_biexp")
                                                     
   

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.

@mikejiang
Copy link
Member

You are right, there seems to be no precise mapping between these two transformations, I am will rollback the previous commit.
Also there are some regressions on ggplot side recently. Let's wait for it's patched release to see if that fixes things.

mikejiang pushed a commit that referenced this issue Dec 8, 2022
@phauchamps
Copy link
Author

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

@phauchamps
Copy link
Author

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

@teunbrand
Copy link

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:

scale_y_logicle <- function(..., w = 0.5, t = 262144, m = 4.5, a = 0){
myTrans <- logicle_trans(w = w, t = t, m = m, a = a)
scale_y_continuous(..., trans = myTrans)
}

And while it seems it would be best to fix this in the upstream package that defines logicle_trans(), it seems that the issue can be worked around in {ggcyto} as well, by overruling the transformation's domain.

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,
Teun

@mikejiang
Copy link
Member

@teunbrand I hesitate to change that default behavior of logicle transformation function, i.e. infinite values are not valid input.
so I adopted your second approach. This seems to work properly.

@phauchamps thank you so much spending time getting to the bottom of this issue. Let me know if the latest patch works for you

@phauchamps
Copy link
Author

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

@mikejiang
Copy link
Member

pushed

@phauchamps
Copy link
Author

Got if from BiocManager::install() in release version (3.16).
Can't see it (yet) though in devel version (3.17).

@mikejiang
Copy link
Member

develop branch is lagging behind, see the snapshot time here http://bioconductor.org/checkResults/3.17/bioc-LATEST/
I think it should appear by the end of today or tomorrow

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

No branches or pull requests

5 participants