JuliaMath / FixedPointNumbers.jl

fixed point types for julia
Other
79 stars 34 forks source link

Enable package extension for Statistics #277

Closed hyrodium closed 2 months ago

hyrodium commented 4 months ago

This PR reverts #255.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.05%. Comparing base (cf4b0c3) to head (cc5d38b).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #277 +/- ## ======================================= Coverage 97.05% 97.05% ======================================= Files 6 7 +1 Lines 782 782 ======================================= Hits 759 759 Misses 23 23 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kimikage commented 3 months ago

This seems more elegant than the use of Requires.jl with respect to Stdlib.

On the other hand, in the first place, this workaround uses a non-public function Statistics._mean_promote. Until now, this has been acceptable because the version of Statistics.jl was synchronized with Julia's version upgrades.

Seems to me we need to revisit the workaround itself. Of course, that should be separate from this PR.

kimikage commented 3 months ago

This change is not so complicated, and the intent is clear. If we decide to merge this PR, however, making the simple revert commit into master might make the tracking easier. This is just a little reminder for the hygiene of commit-graph, because PR #278 also makes changes to Project.toml.

johnnychen94 commented 3 months ago

Sorry, I missed this PR.

Adding extension support is a good change, but it requires Julia 1.9 to work.

For the very fundamental packages such as FixedPointNumbers, keeping a broad range of compatibility support is necessary. It's necessary to keep it compatible with the current LTS version, i.e., Julia 1.6. The current FixedPointNumbers is still compatible with Julia 1.0, which is more than great.

Thus, we still need to preserve the old codes, still need to install Requires (but conditionally loading it depending on the actual Julia versions).

Ref: https://pkgdocs.julialang.org/v1/creating-packages/#Backwards-compatibility

kimikage commented 3 months ago

@johnnychen94 You are absolutely right. We might need Requires.jl (perhaps for CheckedArithmetic?).

But for the case of Statistics, it has been a stdlib and should work as before. (Conversely, the "new” Statistics.jl will basically not be introduced into the old julia.)

johnnychen94 commented 3 months ago

Adding extension support is a good change, but it requires Julia 1.9 to work.

Sorry, I misunderstood the code. The current version is compatible with old Julia versions by falling back to a direct include rather than the lazy @require.

~My two cents is to still use the lazy @require because it's what we expected in #255.~ (never mind, I just realized that #255 is a patch for Julia 1.9, for which version extension is supported...)

kimikage commented 3 months ago

@hyrodium Could you rebase (and tweak) this?

kimikage commented 3 months ago

cf. https://github.com/JuliaStats/Statistics.jl/issues/165

kimikage commented 3 months ago

@hyrodium Thank you for rebasing. Is this ready to merge? Or do you plan to revise it (regarding my comments)? I will merge this PR once I get the go-ahead.

kimikage commented 2 months ago

@hyrodium Thank you for addressing this issue!