-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add option suppress_default_hovertext for plot_method = "ggplot" #301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Braces and logical comments apply to other parts of the diff too
Would suggest that this should also work with plot method set to plotly
R/heatmaply.R
Outdated
@@ -887,6 +890,7 @@ heatmaply.heatmapr <- function(x, | |||
point_size_name = "Point size", | |||
label_format_fun = function(...) format(..., digits = 4), | |||
custom_hovertext = x[["matrix"]][["custom_hovertext"]], | |||
suppress_default_hovertext = F, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
False not f
R/plots.R
Outdated
col, ": ", mdf[[2]], "<br>", | ||
val, ": ", label_format_fun(mdf[[3]]) | ||
) | ||
if(!suppress_default_hovertext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the same brace style as the rest of the code
R/plots.R
Outdated
@@ -95,7 +101,13 @@ ggplot_heatmap <- function(xx, | |||
} | |||
} | |||
if (!is.null(custom_hovertext)) { | |||
mdf[["text"]] <- paste0(mdf[["text"]], "<br>", custom_hovertext) | |||
if(!suppress_default_hovertext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invert this if/else
@alanocallaghan at least with v1.5, when using custom_hovertext with the plot_method = plotly method the default is not shown. |
What I mean is that the behavior should be equivalent |
This currently fails with: heatmaply(mtcars, suppress_default_hovertext=TRUE)
Error in ggplot_heatmap(data_mat, row_text_angle, column_text_angle, scale_fill_gradient_fun, :
object 'suppress_default_hovertext' not found If I add the arg to ggplot_heatmap, I get: heatmaply(mtcars, suppress_default_hovertext=TRUE)
Error in `.data$text`:
! Column `text` not found in `.data`.
Run `rlang::last_trace()` to see where the error occurred. Also hovertext is currently broken generally for plot_method="plotly" so I'd want to resolve that before merging |
Thanks @alanocallaghan. I added the arg to my fork and it works for me if I install heatmaply from it. Could there be other changes after merging? |
Please also add yourself as contributor to the DESCRIPTION file.
T
…On Sun, 24 Dec 2023, 20:07 mcsimenc, ***@***.***> wrote:
Thanks @alanocallaghan <https://github.com/alanocallaghan>. I added the
arg to my fork and it works for me if I install heatmaply from it. Could
there be other changes after merging?
—
Reply to this email directly, view it on GitHub
<#301 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHOJBVPA2J5GWVFFYSLPXTYLBVNNAVCNFSM6AAAAABBALV3YWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRYGU3DSNBXGU>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Hi @mcsimenc |
Hey guys, I'm so sorry about the lag. I've been so busy with work and home life and haven't returned to this. They are simple changes and I will try to get to it this weekend! |
No worries, we all get it :)
Thanks 👍🙏
…On Mon, 20 May 2024, 22:13 mcsimenc, ***@***.***> wrote:
Hey guys, I'm so sorry about the lag. I've been so busy with work and home
life and haven't returned to this. They are simple changes and I will try
to get to it soon!
—
Reply to this email directly, view it on GitHub
<#301 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHOJBR2AB52W7DTT4WW7QTZDJRN7AVCNFSM6AAAAABBALV3YWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRRGIYTOMZZGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Hi @mcsimenc - any update on this? (I'd be happy for you to finish this so I'd include it in the next release) T |
Hi Tal and Alan, thanks for your patience. I made the changes and am able to run
I'll be available over the next two weeks if any other changes need to be made. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revise a tiny bit and then resubmit. Thanks!
DESCRIPTION
Outdated
@@ -9,6 +9,7 @@ Authors@R: c( | |||
person("Jonathan", "Sidi", email = "[email protected]", comment = "https://github.com/yonicd",role = "ctb"), | |||
person("Jaehyun", "Joo", comment = "https://github.com/jaehyunjoo",role = "ctb"), | |||
person("Yoav", "Benjamini", email = "[email protected]",role = "ths")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a comma
person("Yoav", "Benjamini", email = "[email protected]",role = "ths")) | |
person("Yoav", "Benjamini", email = "[email protected]",role = "ths")), |
R/heatmaply.R
Outdated
@@ -571,6 +571,7 @@ heatmaply.default <- function(x, | |||
label_format_fun = function(...) format(..., digits = 4), | |||
labRow = NULL, labCol = NULL, | |||
custom_hovertext = NULL, | |||
suppress_default_hovertext = F, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use FALSE instead of F
suppress_default_hovertext = F, | |
suppress_default_hovertext = FALSE, |
R/plots.R
Outdated
@@ -95,7 +103,11 @@ ggplot_heatmap <- function(xx, | |||
} | |||
} | |||
if (!is.null(custom_hovertext)) { | |||
mdf[["text"]] <- paste0(mdf[["text"]], "<br>", custom_hovertext) | |||
if (suppress_default_hovertext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All other places you've used ! suppress_default_hovertext, it might be worth be consistent. But your choice on this one.
Hi @mcsimenc Thanks! p.s.: FYI that once you finish I'll squash your commits into a single one, so no worries on making many commits. |
Got it. I may have misunderstood Alan's comment about inverting the true/false. I switched it to use the negation and made the other changes. Thank you Tal |
I removed an extra paren in the DESCRIPTION file that caused R-CMD-check to fail |
Great, I'm running the tests again. |
It's almost there, but there is still an error. Here is one of the errors:
|
Ok, @param added. |
Hi @mcsimenc I see there is still an issue with the rd file. Could you please resolve it?
|
Thanks. |
… and modified param descriptions in R files to match.
Ok, Tal, try again! |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #301 +/- ##
==========================================
- Coverage 91.67% 91.66% -0.02%
==========================================
Files 9 9
Lines 1346 1356 +10
==========================================
+ Hits 1234 1243 +9
- Misses 112 113 +1 ☔ View full report in Codecov by Sentry. |
Almost there.
|
Make sure the relative location of the roxygen2 param is the same as in the code. |
@mcsimenc
|
Hi Tal, yes trying to resolve it. The params are the same in the docs and the function def, and I ran roxygenise(). Should that do it? |
Hi,
Did you push your updates? The latest check I ran still doesn't seem to
have resolved the issue.
…On Tue, 16 Jul 2024, 2:14 mcsimenc, ***@***.***> wrote:
Hi Tal, yes trying to resolve it. The params are the same in the docs and
the function def, and I ran roxygenise(). Should that do it?
—
Reply to this email directly, view it on GitHub
<#301 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHOJBQGCPGVQMKZDPW6CGTZMRJVRAVCNFSM6AAAAABBALV3YWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRZGYYDCNRXGE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
No, I didn't, until just now.
Thanks Tal
…On Tue, Jul 16, 2024 at 1:18 AM Tal Galili ***@***.***> wrote:
Hi,
Did you push your updates? The latest check I ran still doesn't seem to
have resolved the issue.
On Tue, 16 Jul 2024, 2:14 mcsimenc, ***@***.***> wrote:
> Hi Tal, yes trying to resolve it. The params are the same in the docs
and
> the function def, and I ran roxygenise(). Should that do it?
>
> —
> Reply to this email directly, view it on GitHub
> <#301 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AAHOJBQGCPGVQMKZDPW6CGTZMRJVRAVCNFSM6AAAAABBALV3YWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRZGYYDCNRXGE>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#301 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABYL2ZT6LX2GA2DYWB5KJBLZMTJOBAVCNFSM6AAAAABBALV3YWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZQGI4TSOJSGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Congrats 👏🎉 |
Awesome! |
closes #300