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

Problems with conversion of Unsigned to RGB #118

Open
mateuszbaran opened this issue Aug 23, 2021 · 5 comments
Open

Problems with conversion of Unsigned to RGB #118

mateuszbaran opened this issue Aug 23, 2021 · 5 comments

Comments

@mateuszbaran
Copy link

I've accidentally been using Base.convert method from this package that converts Unsigned to RGB. This is type-unstable (which is easy to fix) but it also looks like type piracy, so this method likely shouldn't even be in this package. Here is that method:

function Base.convert(::Type{RGB}, h::Unsigned)
mask = 0x0000FF
RGB([(x & mask) / 0xFF for x in (h >> 16, h >> 8, h)]...)
end
. What do you think should be done about it?

@t-bltg
Copy link
Member

t-bltg commented Aug 25, 2021

@BeastyBlacksmith, do you know why this is exposed in Base ?

@BeastyBlacksmith
Copy link
Member

This predates my time quite a bit. But I agree, that this is type piracy and we should get rid of it.
One just has to be careful with this, since it is unclear where this is used. It might not only affect Plots, if we remove it.

@t-bltg
Copy link
Member

t-bltg commented Aug 25, 2021

This predates my time quite a bit. But I agree, that this is type piracy and we should get rid of it.
One just has to be careful with this, since it is unclear where this is used. It might not only affect Plots, if we remove it.

Thanks.

@mateuszbaran you can make a PR changing this to an internal method, but you'll have to run Plots.jl test suite, checking that this does not break anything.
Someone should also investigate where this method is used in the JuliaPlots ecosystem, to find potential breaks.

@BeastyBlacksmith
Copy link
Member

Here is a list of all dependents: https://juliahub.com/ui/Packages/PlotUtils/YveHG/1.0.11?t=2

@mateuszbaran
Copy link
Author

That's quite a lot of dependent packages. I could check if removing it breaks Plots.jl but going through all of these packages is a bit too much 😅 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants