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

Use tuples instead of StaticArrays #42

Closed
rafaqz opened this issue Sep 10, 2020 · 8 comments · Fixed by #49
Closed

Use tuples instead of StaticArrays #42

rafaqz opened this issue Sep 10, 2020 · 8 comments · Fixed by #49

Comments

@rafaqz
Copy link
Collaborator

rafaqz commented Sep 10, 2020

As @KristofferC just mentioned in #33 we could use a tuple instead of a Vector for colors chemes, and remove the StaticArrays.jl dependency.

Is there any reason colors need to be in a Vector ? I (probably wrongly) assumed there was when I added the dependency to StaticArrays.jl to speed up color-scheme indexing. Tuples are cleaner and would improve package load time a lot, and the load time of other packages like Gaston.jl.

I'll write the PR if there is no problem with making the change.

@cormullion
Copy link
Member

cormullion commented Sep 10, 2020

Sounds good to me - it's a long time since I made this package (Julia v0.4) ... :) Make sure you don't break the entire plotting ecosystem... :)

@rafaqz
Copy link
Collaborator Author

rafaqz commented Sep 10, 2020

Eeeek that was my worry... We would have to test all the dependent packages before merging.

@mbaz
Copy link

mbaz commented Sep 10, 2020

Maybe this change merits a major, breaking release? That way you wouldn't (technically) break any package that depends on ColorSchemes. I can't speak for everybody but I'd welcome the improved load times.

@KristofferC
Copy link

Why would this be a breaking change? Are people doing math with the color vector? It's just a list of colors.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Oct 20, 2020

This mostly works and package load time is less than half of what it is with StaticArrays, but documentation breaks
https://travis-ci.com/github/rafaqz/ColorSchemes.jl/jobs/402345022

I think using Tuples loses a bunch of show methods for vectors of colors. So they will also no longer show in atom etc. I'm not sure how much work that is to fix.

@cormullion
Copy link
Member

I don't think there are any tests for show at the moment... :( I could fix the documentation to not automatically insert a colorscheme by default, but it would be better if there was some kind of show method perhaps.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Oct 20, 2020

I really like seeing the colorscheme in atom - I guess it would mean adding a method here in Colors.jl?
https://github.com/JuliaGraphics/Colors.jl/blob/0890b410940fa9645496a394e5c449ef7104769a/src/display.jl#L26

The AbstractArray interface does seem kinda handy ;) I'm wondering if it's worth losing the simplicity of that for 1 second of start time...

@rafaqz
Copy link
Collaborator Author

rafaqz commented Oct 20, 2020

Maybe ColorScheme could itself be in <: AbstractVector and that would be solved. It already defines getindex the whole abstract array interface, it may as well be an AbstractVector

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 a pull request may close this issue.

4 participants