-
Notifications
You must be signed in to change notification settings - Fork 24
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
Making function value types the same as input eltype #89
Conversation
Codecov Report
@@ Coverage Diff @@
## master #89 +/- ##
==========================================
+ Coverage 85.19% 85.25% +0.05%
==========================================
Files 75 75
Lines 2107 2102 -5
==========================================
- Hits 1795 1792 -3
+ Misses 312 310 -2
Continue to review full report at Codecov.
|
I am not completely convinced by #74 but this might be a good idea. I will have to look through it a bit more carefully beginning next week. If possible, it would be really nice to test with weird things like dual numbers. I'm afraid this could break some automatic differentiability (without looking at the code). |
I agree that we could test those situations. But this PR shouldn’t (and hopefully currently doesn’t) affect much the manipulation of |
f4ffbd9
to
2cb5e51
Compare
0d2348a
to
d522724
Compare
d522724
to
041eea3
Compare
@mfalt do you have any further concern with this? Otherwise I would like to get this merged soon. The idea in #74 makes sense to me: for any function
|
Sorry for not looking at this earlier. I think it looks very reasonable, especially since the conversions are only applied to the parameters. I think most of this problem could be solved by making sure that all defaults are low enough, for example integers, but it would leave no guarantees like this does. The parameters seems to be mostly restricted to Reals anyway, so there might not be any (interesting) existing cases where the conversion fails (for example the parameter being a dual number). The only case I can come up with that breaks is if the parameter is a float and the user tries to call the function or prox with integer values, but that's not a big deal. My general feeling is that if a user creates a function with Float64 parameters, then the user is also happy with receiving Float64 outputs. But I have no strong opinions, so feel free to merge. I definetely don't think it's bad, maybe slightly unnecessary. But I also don't see that the PR would cause any problems. |
Ok, I thought you referred to the input, not parameters, when mentioning dual numbers. Yes, they are mostly restricted to Real for now: that might be something that we want to remove, in which case we can rethink this. The idea of having low enough default parameters may be ideal then.
For that, would it make sense to restrict |
I think it's better to keep the restriction on x as it is in this PR. I would rather get accuracy errors in some special cases than being unable to call the function for some types where it would actually work. Edit: e.g FixedPoint or Rational |
@mfalt you're right. In any case, I've given your previous comments some thought (thanks for those) and I'm convinced that all this casting of function parameters may not be the best solution. I'm working instead towards lowering the type of their defaults, as you suggested, and it appears much cleaner to me. With that, the convention on input vs function type would be
Similarly for My available time is what it is, but I'll update this PR as soon as possible. |
I think that sounds like a good compromise, hopefully it's not too tricky to implement. Unfortunately I don't currently have any spare time to spend on this project. |
Done: with the latest update, now the changes to the package should really be minor (albeit the diff is long). Most of the elaborate changes are really in the tests. Kindly asking @nantonel to also take a look :-) disable white-space changes for easier review! |
I skimmed through the changes and it looks really good. We should just make sure to tag it 0.13 some of the changed type parameters might be breaking for someone, although it seems unlikely. Is there a roadmap for bumping the version to 1.0? It feels the package is very stable by now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All minor comments (just style). Feel free to ignore them. Great work! 😄
end | ||
end | ||
|
||
function gradient!(y::AbstractArray{T}, f::HuberLoss, x::AbstractArray{T}) where T <: Union{Real, Complex} | ||
function gradient!(y::AbstractArray{T}, f::HuberLoss, x::AbstractArray{T}) where {R <: Real, T <: Union{R, Complex{R}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T<:RealOrComplex{R}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! So I’ve actually changed my mind about that: RealOrComplex{R}
is 17 characters while Union{R, Complex{R}}
is 20, not much of a shorthand plus the latter is straight from Base
and fully explicit
end | ||
return v | ||
end | ||
|
||
function prox!(y::AbstractArray{T}, f::HuberLoss, x::AbstractArray{T}, gamma::Real=1.0) where T <: Union{Real, Complex} | ||
function prox!(y::AbstractArray{T}, f::HuberLoss, x::AbstractArray{T}, gamma::R=R(1)) where {R <: Real, T <: Union{R, Complex{R}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T<:RealOrComplex{R}
@@ -76,11 +76,11 @@ fun_dom(f::HuberLoss) = "AbstractArray{Real}, AbstractArray{Complex}" | |||
fun_expr(f::HuberLoss) = "x ↦ (μ/2)||x||² if ||x||⩽ρ, μρ(||x||-ρ/2) otherwise" | |||
fun_params(f::HuberLoss) = string("ρ = $(f.rho), μ = $(f.mu)") | |||
|
|||
function prox_naive(f::HuberLoss, x::AbstractArray{T}, gamma::Real=1.0) where T <: Union{Real, Complex} | |||
y = (1-min(f.mu*gamma/(1+f.mu*gamma), f.mu*gamma*f.rho/(norm(x))))*x | |||
function prox_naive(f::HuberLoss, x::AbstractArray{T}, gamma::R=R(1)) where {R <: Real, T <: Union{R, Complex{R}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T<:RealOrComplex{R}
This PR aims at addressing #74, as well as factoring a few tests a bit better as a side effect.Essentially, this enforces the following
for any
f::ProximableFunction
, wheneverf
is constructed with default arguments, or with parameters based off real(eltype(x)).Description of the changes
The above is achieved by:
prox_test
utility function (this was there already, but tests were coveringFloat64
only AFAIU)Float32
andFloat64
, or the corresponding complex types (this required changes to test_calls.jl, and I took the chance to also reorganize that script quite a bit)Remarks
In principle, one could additionally require thereal(eltype(x))
to be the same as the real type underlying the parameters off
(such as coefficients), and avoid the need of doing type conversion at all. However, this creates issues when the parameters have defaults: for example, the coefficientlambda
inf = NormL1()
defaults to1.0::Float64
(on 64-bit machines, I guess), so by defaultf
would only be applicable toAbstractArray{Float64}
orAbstractArray{Complex{Float64}}
, and one would need to explicitly constructf = NormL1(Float32(1))
to be able to use it withx::Array{Float32}
.Note also that not all test cases have been enabled with multiple real types just yet, I'm opening this PR nevertheless to get early feedback.@mfalt I remember we discussed this in the past: I'd appreciate inputs from you on this