-
Notifications
You must be signed in to change notification settings - Fork 370
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
remove deprecations and CategoricalArrays.jl dependency #2554
Conversation
It seems that removing CategoricalArrays.jl dependency does not have much impact on compilation times now: Master
This PR:
except for package loading time (which was expected to decrease). @timholy - where would you expect the biggest gains. Thank you! CC @nalimilan |
First, CategoricalArrays no longer invalidates much stuff, so there may be no urgency in getting rid of it. Version 0.9 only invalidates 212 methods, and most of these are probably inconsequential. Second, when you do invalidate stuff, it's often the things that build on you, not you yourself, that are the big winners. For example, I bet that if Gadfly added a precompile file (using the tricks in SnoopCompile) they'd now have vastly more success. |
Thank you for the explanation.
As you have commented on Slack - we have already removed the dependency in 0.22 release. Here we only remove the actual Still - even if CategoricalArrays.jl is much more compiler friendly now (which I agree it is) it is better anyway to have no dependence between the packages and make them interoperate via DataAPI.jl interface. |
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@nalimilan - this should be good for a review |
updated after the review |
@nalimilan - I have just checked that we have a significant regression on Julia nightly (the timings are the for the same operations as ones above):
|
Wow, with 99% spent in compilation time, that's a serious regression in Julia. Can you file a bug there? |
Thank you! |
Is there a reason |
This must have been a mistake when rebasing PR. It should be only test dependency. I will fix it. |
see #2646 (as I am not 100% certain about Project.toml semantics) |
No description provided.