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

CompatHelper: bump compat for "Colors" to "0.11" #1369

Merged

Conversation

github-actions[bot]
Copy link
Contributor

This pull request changes the compat entry for the Colors package from 0.9 to 0.9, 0.11.

This keeps the compat entries for earlier versions.

Note: I have not tested your package with this new compat entry. It is your responsibility to make sure that your package tests pass before you merge this pull request.

@codecov-io
Copy link

codecov-io commented Dec 19, 2019

Codecov Report

Merging #1369 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1369   +/-   ##
======================================
  Coverage    90.6%   90.6%           
======================================
  Files          38      38           
  Lines        3961    3961           
======================================
  Hits         3589    3589           
  Misses        372     372

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb210ab...22d25fa. Read the comment docs.

@bjarthur bjarthur merged commit 0ec5523 into master Dec 19, 2019
@bjarthur bjarthur deleted the compathelper/new_version/2019-12-19-02-37-25-642-2277421508 branch December 19, 2019 12:25
@kimikage
Copy link

@bjarthur, how did you confirm the compatibility with Colors.jl v0.11? If you didn't confirm it, this change (or pressing the merge button carelessly) is very dangerous.

@Mattriks
Copy link
Member

I note that this PR was opened and tested before Compose.jl's Colors 0.11 compat was triggered (GiovineItalia/Compose.jl#375), I'm guessing that the above Travis test used Colors 0.9.6 not 0.11. Compare with the more recent PRs #1370 and #1371 which both fail on the testscript "discrete_color_hue.jl" for Julia 1.0.5

@kimikage
Copy link

kimikage commented Dec 22, 2019

I am pending submitting an issue because I have not yet identified the cause. (I think this is not a problem of Gadfly's code base.)

However, I know some possible causes. The significant difference between Colors v0.9.6 and v0.10 which may cause the segmentation fault, is the precompilation (cf. JuliaGraphics/Colors.jl#370)

plot(ones(10,3).*[1 2 3], x=Row.index, y=Col.value, color=Col.index,
Geom.line, Scale.color_discrete_hue(x->range(RGB(1,0,0), stop=RGB(0,0,1), length=x)))

RGB(1,0,0) generates RGB{N0f8}(1,0,0). N0f8 is defined in FixedPointNumbers.
If we do not use RGB{N0f8} (e.g. using RGB{Float32}), we will get different results.

Edit:

FixedPointNumbers ColorTypes Test
v0.6.1 (w/o pc) v0.8.0 (w/o pc) ✔️
v0.6.1 (w/o pc) v0.8.1 (w/ pc) ❌ segfault
v0.7.0 (w/ pc) v0.8.0 (w/o pc) ❌ segfault
v0.7.0 (w/ pc) v0.8.1 (w/ pc) ❌ segfault

cf. JuliaLang/julia#34121

@tlnagy
Copy link
Member

tlnagy commented Dec 22, 2019

I'm guessing that the above Travis test used Colors 0.9.6 not 0.11.

I can confirm that this is true. I merged a PR to revert this change over at #1372, despite tests failing since it's simply returning to the state prior to this merge.

@tlnagy
Copy link
Member

tlnagy commented Dec 22, 2019

I rebased #1371 on #1372 and I'm letting the tests run (everything should pass) and we need to merge it and it should get us some time to work out the issues with Colors v0.10+. That PR will pin Colors back to the v0.9 branch in Gadfly's dependency list until we resolve this.

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

Successfully merging this pull request may close these issues.

5 participants