Skip to content

Conversation

@torfjelde
Copy link
Contributor

@torfjelde torfjelde commented Jul 10, 2021

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 👍

Return the logarithm of [multivariate gamma function](https://en.wikipedia.org/wiki/Multivariate_gamma_function) ([DLMF 35.3.1](https://dlmf.nist.gov/35.3.1)).
"""
function logmvgamma(p::Int, a::Real)
Copy link
Contributor Author

@torfjelde torfjelde Jul 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for this, i.e. test/misc.jl:45, is currently failing because the numerical accuracy is slightly different than logbeta. I'm very confused as to why it started failing now as the constants aren't even being used in logmvgamma.

This came up before I made the change to use logπ btw; that was an attempt to fix it.

@devmotion
Copy link
Member

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
Copy link
Contributor Author

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 👍

@torfjelde
Copy link
Contributor Author

Also should prob wait for JuliaMath/SpecialFunctions.jl#333 since otherwise bumping LogExpFunctions.jl to 0.3 will lead to a downgrade of SpecialFunctions.

@devmotion
Copy link
Member

SpecialFunctions 1.6.1 supports LogExpFunctions 0.3.

@devmotion devmotion closed this Aug 11, 2021
@devmotion devmotion reopened this Aug 11, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2021

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
Copy link
Contributor Author

Aaaaah you're back my love ❤️

Should we bump the compat entry for SpecialFunctions then?

@devmotion
Copy link
Member

Should we bump the compat entry for SpecialFunctions then?

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

@torfjelde
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Haha, of course; nevermind me! 🙃

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
Copy link
Member

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 #118 (comment) - currently both LogExpFunctions (0.2) and IrrationalConstants define and export these constants.

@devmotion
Copy link
Member

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 added a commit that referenced this pull request Aug 13, 2021
#122)

Co-authored-by: Tamas K. Papp <tkpapp@gmail.com>
Co-authored-by: Tor Erlend Fjelde <tor.erlend95@gmail.com>
@devmotion
Copy link
Member

Fixed and extended by #122.

@devmotion devmotion closed this Aug 13, 2021
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.

3 participants