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

Switch to Colors package (do not merge yet) #667

Merged
merged 3 commits into from
Aug 25, 2015
Merged

Switch to Colors package (do not merge yet) #667

merged 3 commits into from
Aug 25, 2015

Conversation

timholy
Copy link
Collaborator

@timholy timholy commented Aug 24, 2015

Replacement for #664. Do not merge until C-day.

The last commit is mostly for fun, experimenting with whether we can use more concretely-typed containers. This represents what should be a path towards performance improvements for Gadfly (but I haven't tested). If done well, the only potential cost I'm aware of might be increased compilation times under some circumstances. (EDIT: meaning, it might have to compile multiple type-stable versions of some methods, whereas formerly it used only one type-unstable version.)

This uses function barriers to build containers that may, under
favorable circumstances, use a leaftype for their eltypes.
For Colors, it also starts trying to more "input santizing" at the
beginning (in the constructor) to avoid passing type-instability
down to lower levels.
@timholy
Copy link
Collaborator Author

timholy commented Aug 24, 2015

Replaced by #667.


# Allow users to supply strings without deprecation warnings
# Note this strips any transparency; is this desirable?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcjones, is this what you want here? Or is any Colorant OK?

@timholy
Copy link
Collaborator Author

timholy commented Aug 24, 2015

Again, the test failure on release is not anything to worry about (yet), because the tagged version of Cairo forces Color to load and that causes confusion with Colors. On nightly, that's a very odd precompilation failure, one I once saw on my own machine but which went away with a newer version of julia (so I just assumed it was a bug that had already been fixed). CCing @vtjnash and @stevengj.

@stevengj
Copy link

@timholy, it looks like JuliaLang/julia#12508: DataArrays is not marked with __precompile__() and is apparently loaded (in non-precompiled form) by something in the package-test framework(?) before Gadfly and hence fails to load in its precompiled form when Gadfly is precompiled.

Solution: put __precompile__() in DataArrays.jl (assuming it is precompile-safe).

(You never saw it again because it only happens the first time something is precompiled.)

@stevengj
Copy link

Some of the test files have using RDatasets, DataArrays, Gadfly. Put Gadfly first and the problem should go away.

(But in general, whenever you are using a package X and are precompiling, it is best practice to file a PR/issue and get __precompile__() merged in package X. Not only will that avoid this problem, but it also verifies with the upstream package author that X is indeed precompilable.)

@timholy
Copy link
Collaborator Author

timholy commented Aug 24, 2015

Thanks for looking, @stevengj!

@timholy
Copy link
Collaborator Author

timholy commented Aug 24, 2015

@dcjones, if you need more time to review, I will push C-day back. Just let me know.

@dcjones
Copy link
Collaborator

dcjones commented Aug 24, 2015

This looks good to me. Actually, it seems pretty safe to merge this at any time. It works fine with pre-transition Cairo.

@timholy
Copy link
Collaborator Author

timholy commented Aug 25, 2015

Thanks!

Combined testing here: https://travis-ci.org/timholy/Dummy.jl. Assuming this is on-track tomorrow, I'll merge.

timholy added a commit that referenced this pull request Aug 25, 2015
Switch to Colors package (do not merge yet)
@timholy timholy merged commit 7a1c011 into master Aug 25, 2015
@timholy timholy deleted the teh/colors2 branch August 25, 2015 10:07
@protogeezer
Copy link

What is the difference between “pre-transition” Ciaro and what’s coming?

On Aug 24, 2015, at 5:02 PM, Daniel C. Jones notifications@github.com wrote:

This looks good to me. Actually, it seems pretty safe to merge this at any time. It works fine with pre-transition Cairo.


Reply to this email directly or view it on GitHub.

@timholy
Copy link
Collaborator Author

timholy commented Aug 25, 2015

@dcjones was pointing out that he was able to make it work even without switching Cairo to Colors. But both have been switched now.

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.

4 participants