JuliaStats / StatsFuns.jl

Mathematical functions related to statistics.
Other
235 stars 40 forks source link

Depend on IrrationalConstants.jl #118

Closed torfjelde closed 3 years ago

torfjelde commented 3 years ago

Now that IrrationalConstants.jl have been released, the constants in StatsFuns are no longer required.

This is nice since it means that we can take advantange the constants that before were defined in StatsFuns.jl in other packages, without depending on StatsFuns.jl, e.g. some of them would be useful in SpecialFunctions.jl.

EDIT: This has also been done in LogExpFunctions.jl :+1:

devmotion commented 3 years ago

This will lead to ambiguity warnings and redefinitions with LogExpFunctions 0.2, won't it? It seems a bit more consistent also to update LogExpFunctions to 0.3 which depends on IrrationalConstants. Similar to LogExpFunctions maybe one should not reexport the constants anymore and require users to load IrrationalConstants explicitly, @tpapp?

torfjelde commented 3 years ago

This will lead to ambiguity warnings and redefinitions with LogExpFunctions 0.2, won't it? It seems a bit more consistent also to update LogExpFunctions to 0.3 which depends on IrrationalConstants.

Ah yes sorry, checked that LogExpFunctions now depends on IrrationalConstants.jl but forgot to bump the compat version :+1:

torfjelde commented 3 years ago

Also should prob wait for https://github.com/JuliaMath/SpecialFunctions.jl/pull/333 since otherwise bumping LogExpFunctions.jl to 0.3 will lead to a downgrade of SpecialFunctions.

devmotion commented 3 years ago

SpecialFunctions 1.6.1 supports LogExpFunctions 0.3.

codecov-commenter commented 3 years ago

Codecov Report

Merging #118 (23a19e5) into master (56322bb) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #118   +/-   ##
=======================================
  Coverage   28.72%   28.72%           
=======================================
  Files          11       11           
  Lines         369      369           
=======================================
  Hits          106      106           
  Misses        263      263           
Impacted Files Coverage Δ
src/misc.jl 37.50% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 56322bb...23a19e5. Read the comment docs.

torfjelde commented 3 years ago

Aaaaah you're back my love :heart:

Should we bump the compat entry for SpecialFunctions then?

devmotion commented 3 years ago

Should we bump the compat entry for SpecialFunctions then?

For LogExpFunctions you mean, I guess? SpecialFunctions 1.6.1 is already supported.

torfjelde commented 3 years ago

No I mean, this PR will be breaking if you can't use IrrationalConstants.jl, right? So if IrrationalConstants.jl is only compat with SpecialFunctions 1.6.1, then should we lower bound the compat entry for SpecialFunctiosn here in StatsFuns for to 1.6.1?

devmotion commented 3 years ago

You can use IrrationalConstants also with older versions of SpecialFunctions - they don't depend on IrrationalConstants, so you should be able to install both packages just fine.

torfjelde commented 3 years ago

Haha, of course; nevermind me! :upside_down_face:

Sooo are we good then, with the exception of that one test which now seems to fail because of "numerical improvement"?

And what should I do with that test? Just relax the rtol a bit?

devmotion commented 3 years ago

I've seen these test failures before, maybe they are caused by some changes in SpecialFunctions. Can it be reproduced on the master branch?

Otherwise, I think we still have to address https://github.com/JuliaStats/StatsFuns.jl/pull/118#issuecomment-877657834 - currently both LogExpFunctions (0.2) and IrrationalConstants define and export these constants.

devmotion commented 3 years ago

We could also support both LogExpFunctions 0.2 and 0.3, it seems, by changing using LogExpFunctions to an explicit using LogExpFunctions: logit, ... without the constants.

devmotion commented 3 years ago

Fixed and extended by https://github.com/JuliaStats/StatsFuns.jl/pull/122.