search

ropensci / iheatmapr

Complex, interactive heatmaps in R
https://docs.ropensci.org/iheatmapr
Other
267 stars 36 forks source link

Fails with dev scales #59

Closed hadley closed 4 years ago

hadley commented 4 years ago

With test failures like:

── 1. Failure: can add a row barplot to single horizontal heatmap (@test_barplot.R#5)  ─────────────────────────────────
Not equal to reference from `reference/row_barplot_horizontal.rds`.
Component "data": Component 1: Component 6: Component 2: Attributes: < Component 2: Lengths (162, 160) differ (string compare on first 160) >
Component "data": Component 1: Component 6: Component 2: Attributes: < Component 2: 128 string mismatches >
Component "data": Component 1: Component 6: Component 2: 6 string mismatches
AliciaSchep commented 4 years ago

Thanks for the notice, will look into this and how to fix.

AliciaSchep commented 4 years ago

Seems like there are two issues, (1) This package use scales::muted to get default colors (function now gives slightly different result), which was pretty silly choice, can easily change to just use the output of the function, (2) the results of scales::col_numeric have changed. Will need to think about how to adjust this one... right now the tests look for entire charts to be recreated exactly, which is fairly brittle. Probably good idea to rework those expectations to look for an approximately equal json-as-list representation of the chart. Wondering if there is an existing "approximately equal" for colors?

hadley commented 4 years ago

One option might be to use vdiffr? That's what we use for testing ggplot2.

AliciaSchep commented 4 years ago

Thanks for the suggestion, I think in long run switching to use vdiffr would be a good idea, but I think it would first require enabling outputting an svg (also a worthwhile feature in its own right). Will try going with a work-around (checking scales package version, and having two different versions of output) for now.

AliciaSchep commented 4 years ago

I have submitted to cran now an updated version that should work with both old & new versions of scales (using a hacky approach of looking for package version). Will close this issue once that is up on cran, and will add a new issue for better testing approach.

hadley commented 4 years ago

Thanks!

mschilli87 commented 4 years ago

@AliciaSchep:

I have submitted to cran now an updated version...

Would you mind tagging/releasing this version on github, too? (see https://github.com/ropensci/iheatmapr/issues/44).

AliciaSchep commented 4 years ago

@mschilli87 I will tag/release once it is accepted on CRAN.

(Ran into an issue where there was a pre-existing issue with the package on the 'noLD' platform. The fix for this scales issue actually ended up helping a bit for that issue, but did not fully resolve it. Have a fix now and should submit a new release soon).

AliciaSchep commented 4 years ago

Updated version on CRAN now and made a tag/release on github.