Skip to content
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

Use InverseFunctions #52

Closed
devmotion opened this issue Oct 9, 2021 · 13 comments · Fixed by #56
Closed

Use InverseFunctions #52

devmotion opened this issue Oct 9, 2021 · 13 comments · Fixed by #56

Comments

@devmotion
Copy link
Member

devmotion commented Oct 9, 2021

The package InverseFunctions.jl will be available in the registry soon and contain inverse + definitions for functions in base. If other packages such as LogExpFunctions implement this interface we could replace GPLikelihoods.inverse with InverseFunctions.inverse and remove our definitions.

@devmotion devmotion changed the title Use InvertibleFunctions Use InverseFunctions Oct 10, 2021
@devmotion
Copy link
Member Author

Note: We can't switch before LogExpFunctions and StatsFuns support it though (if we don't want to introduce type piracy).

@oschulz
Copy link

oschulz commented Nov 7, 2021

Do we already have something pending on StatsFuns?

@devmotion
Copy link
Member Author

No, I didn't push this issue the last weeks. I am up for reviewing a PR though.

@oschulz
Copy link

oschulz commented Nov 7, 2021

I am up for reviewing a PR though.

Sure, I can do one. Which functions in StatsFuns should be targeted?

@oschulz
Copy link

oschulz commented Nov 7, 2021

I guess the relevant functions in StatsFuns are the cdf, ccdf, logcdf and logccdf and their inverses, right (since the log/exp functions live in LogExpFunctions now)?

@devmotion
Copy link
Member Author

Yes, I guess everything that's defined in StatsFuns and not in LogExpFunctions.

@oschulz
Copy link

oschulz commented Nov 7, 2021

Uh, I was just about to get started, and then realized that those function do of course all take multiple arguments - InverseFunctions.inverse doesn't support that (would be tricky to define what inverse means). We could define InverseFunctions.inverse (and even ChangesOfVariables.with_logabsdet_jacobian if we want) for Base.Fix1(Distributions.cdf) and so on in a generic fashion, though.

Or would it be fine to just do normcdf & friends?

What does GPLikelihoods need, here?

@oschulz
Copy link

oschulz commented Nov 7, 2021

And I just saw sqrt/square in inverse.jl here. I actually had that in an eary draft of InverseFunctions, but then took it out because Base has no inverse of sqrt and hadn't seen sqrt/square used in variable trafos.

We should add square to InverseFunctions, though, if it's useful here.

@devmotion
Copy link
Member Author

Ah true, yes, it only makes sense for one-argument functions. We only need normcdf and norminvcdf here.

@oschulz
Copy link

oschulz commented Nov 7, 2021

What about sqrt? Is it used, or just in inverse.jl for completeness?

@devmotion
Copy link
Member Author

No, everything in inverse.jl is used. It is needed for the SquareLink (alias and constructor might be removed at some point but we still want to support it).

@oschulz
Copy link

oschulz commented Nov 7, 2021

Ok, then let's just add square to InverseFunctions. Should it be exported, you think?

Also - should we support it in ChangesOfVariables? Would require it to depend on InverseFunctions, but I don't think that should be a problem.

@oschulz
Copy link

oschulz commented Nov 8, 2021

InverseFunctions supports sqrt now (JuliaRegistries/General#48374).

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 a pull request may close this issue.

2 participants