JuliaAI / Imbalance.jl

A Julia toolbox with resampling methods to correct for class imbalance.
https://juliaai.github.io/Imbalance.jl/dev/
MIT License
28 stars 1 forks source link

Revisit Julia 1.6 support #65

Closed ablaom closed 11 months ago

ablaom commented 11 months ago

Currently we have julia="1.6 in the Project.toml. If we are explicitly using Xoshiro RNG's , then we can only support >= 1.7. On the other hand, CI is only testing 1.8 and the latest version (now 1.9).

I think for now we should:

But we could also do both at once.

EssamWisam commented 11 months ago

Alrighty. Working on a small PR after adding some implementation notes in the docs and other improvements. Once done will bump Julia compat to 1.7 and add it to the test matrix.

EssamWisam commented 11 months ago

@ablaom, Okay I am a little confused. Why is the second step needed if we'll bump compat to 1.7? No one with Julia VERSION <= v"1.7" can install Imbalance.jl in this case and Xoshiro (which I do use explictily in algorithms when an integer is given, tests and examples) is available.

ablaom commented 11 months ago

I was only suggesting the bump to 1.7 as a temporary measure.

Longer term, I thought it would be nice to support Julia 1.6 (revert the compat to julia="1.6") because it is the Julia Long Term Support release. (Some research institutions, eg defense-related research, are limited to how often they can update their software stacks, for security reasons. So I think it is a good idea to support the older platform, until Julia itself stops supporting it.) But we can't do this without some kind of fix for the fact that Xoshiro was not introduced until 1.7.

One possiblity is to explicitly introduce version-specific code to switch out Xoshiro for MersenneTwister if VERSION <= V"1.7", and adjust tests if necessary.

Another breaking solution is to always use MersenneTwister and adjust tests if necessary. (BTW, I don't think a breaking release at this early stage is a big deal.)

I thought we might use use Random.default_rng() but that is pre-seeded. Some variations on that idea are not working for me, because copy is not implemented for MersenneTwister in Julia 1.6

Maybe the best solution is to just forget about supporting 1.6 and do something if it is requested. I see SciML has already dumped support for Julia <= 1.9, probably because they like the pkg extension feature.

ablaom commented 11 months ago

Okay, I would like to push a little harder for 1.6 support. See for example this discussion. Also, MLJ supports 1.6 and so the planned integration is currently failing CI.

EssamWisam commented 11 months ago

Okay, I would like to push a little harder for 1.6 support. See for example this discussion. Also, MLJ supports 1.6 and so the planned integration is currently failing CI.

Sorry. I should have said that I have absolutely agreed with your prior message (makes lots of sense to support LTS) and that I scheduled myself to finish working on it today. Other issues besides the RNG popped up and currently dealing with them. In fact, RNG wasn't really hard to solve.

EssamWisam commented 11 months ago

@ablaom Julia 1.6 support is here now. This is the new RNG dosctring:

    "RNG" => """
    - `rng::Union{AbstractRNG, Integer}=default_rng()`: Either an `AbstractRNG` object or an `Integer` 
        seed to be used with `Xoshiro` if the Julia `VERSION` supports it. Otherwise, uses MersenneTwister`.
    """,
ablaom commented 11 months ago

Perfect. Then I guess you can tag a new patch release and close this issue.