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

WIP: support SpecialFunctions 0.8 and 0.7 #119

Closed
wants to merge 3 commits into from
Closed

Conversation

oxinabox
Copy link
Member

When done will close #118
it basically requires solving JuliaPackaging/Requires.jl#3

Right now this just contains the tooling for how to findout package versions and a single example of using it.
Cross-ref:

TODO:

Maybe there is a better way.
Ideally from ChainRules perspective would just put the rules into SpecialFunctions.jl
and then they would always be the right version.
However @ararslan as a maintainer of SpecialFunctions.jl makes the reasonable argument that the SpecialFunctions.jl maintainers should not be obliged to take on responsibility for maintaining the derivatives of the SpecialFunctions.
Particularly while ChainRules is still in alpha.

@ararslan
Copy link
Member

cc @fredrikekre for the Pkg stuff

@fredrikekre
Copy link

fredrikekre commented Oct 14, 2019

Whats wrong with

@static if isdefined(SpecialFunctions, :loggamma)
    @scalar_rule(SpecialFunctions.loggamma(x), SpecialFunctions.digamma(x))
else
    @scalar_rule(SpecialFunctions.lgamma(x), SpecialFunctions.digamma(x))
end

@oxinabox
Copy link
Member Author

A valid point.
TBH I didn't think of it.

It does feel less semantic and kinda adhoc,
but I guess that could be resolved with some coments.
Mostly one will always be able to deferentiate releases via some isdefined.


Unrelated:

  • Right now the conditional is wrong:

    • <=v0.8 should have derivatives for lgamma defined -- it is deprecated not removed.
    • v0.8 should have deriviatives of loggamma defined
  • v0.9 should probably have loggama defined, but maybe should print some kind of warning? idk. Maybe should just assume not breaking. Or should stick to current practices for dependencies as not support things we don't know we can support.

  • At some point in future should remove the lgamma rules, and just have a if ver<v"0.8" then @warn "Current version of ChainRules does not support SpecialFunctions $ver. Please upgrade SpecialFunctions or pin chainRules to v0.x"


I guess that last section has some bearing on the first.

  • It is much easier to reason about SemVer version, so we know when we can removed things and what versions we are supporting.
  • but we could determine the version via ver = isdefined(...) : v"0.7" : isdefined(..) : v"0.8"...

@fredrikekre
Copy link

It is just a more reliable way of doing it; you want to examine the loaded package, not what happened to be found in the env. SpecialFunctions might not even exist there, and if it does, it does not have to correspond to the loaded version.

Right now the conditional is wrong:

  • <=v0.8 should have derivatives for lgamma defined -- it is deprecated not removed.
  • v0.8 should have deriviatives of loggamma defined

I guess you want lgamma derivatives as long as SpecialFunctions.lgamma exist, so something like this, where you would then stop defining the derivative for lgamma the moment it is removed from SpecialFunctions by construction:

@static if isdefined(SpecialFunctions, :loggamma)
    @scalar_rule(SpecialFunctions.loggamma(x), SpecialFunctions.digamma(x))
end
@static if isdefined(SpecialFunctions, :lgamma)
    @scalar_rule(SpecialFunctions.lgamma(x), SpecialFunctions.digamma(x))
end

@oxinabox
Copy link
Member Author

oxinabox commented Oct 14, 2019

You want to examine the loaded package, not what happened to be found in the env. SpecialFunctions might not even exist there, and if it does, it does not have to correspond to the loaded version.

Well that shoots that idea out.

I guess I can just not worry about versions and just worry about what exists.
I wonder if i should make a fancy version of @scalar_rule
like @if_defined_scalarrule.

The corner-case that I do worry about is when we get a circumstance that a functions name stays the same, but its behavour (and thus derivative) changes.
While that happening in a big obvious case is fairly unlikey,
I can certainly imagine that it might happen for say a near a discontinuality, or edge of domain.
A function definition might change from 0 to undefined for example; which would change the definition of its deriviative.

@willtebbutt
Copy link
Member

willtebbutt commented Oct 14, 2019

The corner-case that I do worry about is when we get a circumstance that a functions name stays the same, but its behavour (and thus derivative) changes.
While that happening in a big obvious case is fairly unlikey,
I can certainly imagine that it might happen for say a near a discontinuality, or edge of domain.
A function definition might change from 0 to undefined for example; which would change the definition of its deriviative.

Aren't we at risk of this generally anyway?

edit: to elaborate a little: this is presumably something that a package could pull on us whenever it fancies. I can't see any way to avoid this kind of thing other than with careful unit testing.

@oxinabox
Copy link
Member Author

this is presumably something that a package could pull on us whenever it fancies.

They can use SemVer to tell us, what kind of change they think it is.
Which may or may not be breaking

I can't see any way to avoid this kind of thing other than with careful unit testing.

Even with careful unit testing assuming we find it on any version,
without the ability to specify what code in ChainRules runs for what version of the that package
we can't get it right.

@ararslan
Copy link
Member

Can't we just set the [compat] entry for SpecialFunctions to be 0.8, change the thing to log instead of l, then call it a day?

@nickrobinson251
Copy link
Contributor

Can't we just set the [compat] entry for SpecialFunctions

We do not want to depend on SpecialFunctions, and we cannot set compat for a package we don't depend on, hence using Requires and this dance around Requires not supporting compat bounds

@nickrobinson251
Copy link
Contributor

The corner-case that I do worry about is when we get a circumstance that a functions name stays the same, but its behavour (and thus derivative) changes.

Can we just focus on solving the lgamma -> loggamma case, not solving the general problem of compat-entries for Requires-loaded packages, and just cross our fingers that SpecialFunctions doesn't release a breaking change to the behaviour of loggamma?

@oxinabox
Copy link
Member Author

@fredrikekre 's plan sounds best for now.
I am going to close this PR and make another one based on it

@oxinabox oxinabox closed this Oct 15, 2019
@ararslan
Copy link
Member

We do not want to depend on SpecialFunctions, and we cannot set compat for a package we don't depend on

We can; you can put whatever you want in [compat].

@fredrikekre
Copy link

Registrator will complain, no?

It is also (IMO) kinda weird to demand specific versions of packages while not depending on them.

@ararslan
Copy link
Member

Registrator will complain, no?

I didn't think so, since we have SpecialFunctions in [extras].

@ararslan
Copy link
Member

while not depending on them

Well, we sort of do depend on it, just not in the [deps] sense. We're explicitly using functions from that package in this package.

@fredrikekre
Copy link

So why refuse to add it as a real deps then :)

@ararslan
Copy link
Member

I see no reason not to, tbh

@oxinabox
Copy link
Member Author

oxinabox commented Oct 15, 2019

We only depend on SpecialFunctions.jl in service to SpecialFunctions.jl.
It isn't the case that we need SpecialFunctions.jl to say provide gradients for a Base function.

Pragamatically the reason not to is that it add a binary dependency.
Philosphically the reason not to is that this code belongs in SpecialFunctions.jl
but until the day that ChainRules hits v1.0 and multiple packages are committed to it I am not going to enter that fight.
Particularly since by then we might have a good conditional dependencies story that solves this such that everyone wins.

More likely than adding SpecialFunctions as a dependency to ChainRules is creating a fully standalone glue package for this code.
Do peeps think that is a better idea?

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.

Fixup SpecialFunctions lgamma deprecation
5 participants