Skip to content

Move CategoricalArrays.jl into extension #168

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

Merged
merged 2 commits into from
Jul 21, 2023
Merged

Conversation

MilesCranmer
Copy link
Contributor

@MilesCranmer MilesCranmer commented Jul 21, 2023

Fixes #167. This should be completely backwards compatible. It just decreases load time if the user is not using CategoricalArrays.jl in their application.

Before:

julia> @time_imports using LossFunctions
      1.6 ms  DataAPI
      8.1 ms  Missings
      1.0 ms  Statistics
     62.9 ms  CategoricalArrays
    108.0 ms  LossFunctions

After:

julia> @time_imports using LossFunctions
      0.9 ms  Statistics
      0.4 ms  Requires
      4.0 ms  LossFunctions

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

Merging #168 (b42e0ea) into master (cfcdc6a) will decrease coverage by 0.36%.
The diff coverage is 33.33%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   96.38%   96.03%   -0.36%     
==========================================
  Files           9       10       +1     
  Lines         526      529       +3     
==========================================
+ Hits          507      508       +1     
- Misses         19       21       +2     
Impacted Files Coverage Δ
src/LossFunctions.jl 100.00% <ø> (ø)
src/losses.jl 100.00% <ø> (ø)
ext/LossFunctionsCategoricalArraysExt.jl 33.33% <33.33%> (ø)
src/losses/other.jl 91.11% <33.33%> (ø)

@MilesCranmer
Copy link
Contributor Author

MilesCranmer commented Jul 21, 2023

Tested on 1.9 and 1.6 – everything seems to work.

@juliohm
Copy link
Member

juliohm commented Jul 21, 2023

Can we actually drop versions < v1.9? Do you have requirements to stick to older Julia versions? We are updating the entire stack to require Julia v1.9, it would be nice to do it here already.

@MilesCranmer
Copy link
Contributor Author

MilesCranmer commented Jul 21, 2023

I don't recommend that, as Julia 1.6.* is under long-term support for at least the next few years (LTS happens on a five year cycle): https://julialang.org/downloads/#long_term_support_release. Until a new LTS is released I would advise making key dependencies like this one backwards compatible. You can use Compat.jl for using new features on old Julia versions FYI.

A lot of packages depend on LossFunctions.jl and strive to have backwards compatibility to the LTS version. e.g., if the Julia version is fixed at 1.9 here I would probably need to drop the LossFunctions.jl dependency in SymbolicRegression because some of my users are still using 1.7 (for whatever reason; maybe their cluster uses it for stability?).

@juliohm
Copy link
Member

juliohm commented Jul 21, 2023

It is ok to keep the PR as is 👍🏽 Many ecosystems are already requiring Julia v1.9, so I wondered if we could clean up the Requires.jl boilerplate here as well.

@juliohm juliohm merged commit 1fff2f9 into JuliaML:master Jul 21, 2023
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.

Convert CategoricalArrays.jl to extension?
3 participants