Making function value types the same as input eltype #89

lostella · 2019-12-27T19:30:45Z

~~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

v1 = f(x)
_, v2 = prox(f, x)
_, v3 = gradient(f, x)
typeof(v1) == typeof(v2) == typeof(v3) == real(eltype(x))

for any f::ProximableFunction, whenever f is constructed with default arguments, or with parameters based off real(eltype(x)).

Description of the changes

The above is achieved by:

lowering the default function parameters to integer
asserting the above condition, e.g. in the prox_test utility function (this was there already, but tests were covering Float64 only AFAIU)
enabling some test cases with multiple underlying real types, e.g. using Float32 and Float64, 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 the real(eltype(x)) to be the same as the real type underlying the parameters of f (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 coefficient lambda in f = NormL1() defaults to 1.0::Float64 (on 64-bit machines, I guess), so by default f would only be applicable to AbstractArray{Float64} or AbstractArray{Complex{Float64}}, and one would need to explicitly construct f = NormL1(Float32(1)) to be able to use it with x::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

Project.toml

codecov · 2019-12-27T21:36:08Z

Codecov Report

Merging #89 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
src/functions/indPoint.jl	`78.94% <ø> (ø)`	⬆️
src/calculus/postcompose.jl	`75% <100%> (ø)`	⬆️
src/functions/indBox.jl	`84.37% <100%> (ø)`	⬆️
src/calculus/sqrDistL2.jl	`84.37% <100%> (ø)`	⬆️
src/functions/indAffineDirect.jl	`97.22% <100%> (ø)`	⬆️
src/functions/indHalfspace.jl	`79.16% <100%> (ø)`	⬆️
src/functions/indExp.jl	`84.21% <100%> (-2.64%)`	⬇️
src/functions/sumPositive.jl	`88.88% <100%> (ø)`	⬆️
src/functions/elasticNet.jl	`90.9% <100%> (ø)`	⬆️
src/calculus/precomposeDiagonal.jl	`77.77% <100%> (+1.46%)`	⬆️
... and 16 more

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 6b7f323...0d2348a. Read the comment docs.

mfalt · 2019-12-28T10:15:53Z

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).

lostella · 2019-12-29T18:12:49Z

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 x. For example, no type conversion occurs on x or other arrays derived from x, but only on function parameters as soon as they enter the computational graph.

lostella · 2020-10-22T21:36:02Z

@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 f, if x is a FloatXYZ-based array then f(x) will be of type FloatXYZ. See my previous comment about autodiff:

this PR shouldn’t (and hopefully currently doesn’t) affect much the manipulation of x. For example, no type conversion occurs on x or other arrays derived from x, but only on function parameters as soon as they enter the computational graph

mfalt · 2020-10-22T21:57:38Z

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.

lostella · 2020-10-22T23:24:16Z

(for example the parameter being a dual number).

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.

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.

For that, would it make sense to restrict x to AbstractFloat-based arrays?

mfalt · 2020-10-28T11:56:04Z

For that, would it make sense to restrict x to AbstractFloat-based arrays?

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

lostella · 2020-10-28T12:35:46Z

@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

typeof(f(x)) == real(eltype(x)) whenever f is instantiated with default options, or with parameters based off real(eltype(x))

Similarly for prox. This is a weaker condition than #74, does not involve any type conversion, and is mainly about having test cases that cover the above condition. Also there were still many cases uncovered in tests here.

My available time is what it is, but I'll update this PR as soon as possible.

mfalt · 2020-10-28T18:03:11Z

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.

lostella · 2020-11-01T21:59:32Z

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!

mfalt · 2020-11-02T01:09:33Z

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.
Edit: In fact, it seems like the only open issue that would be breaking enough for is to wait with tagging 1.0 is #16 . I wasn't convinced by it at the time, but it was a while ago since I considered it.

nantonel

All minor comments (just style). Feel free to ignore them. Great work! 😄

src/calculus/distL2.jl

nantonel · 2020-11-02T16:03:43Z

src/functions/huberLoss.jl

    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}}}


T<:RealOrComplex{R}

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

nantonel · 2020-11-02T16:04:08Z

src/functions/huberLoss.jl

    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}}}


T<:RealOrComplex{R}

nantonel · 2020-11-02T16:04:39Z

src/functions/huberLoss.jl

@@ -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}}}


T<:RealOrComplex{R}

src/functions/indExp.jl

src/functions/logisticLoss.jl

src/functions/normL0.jl

src/functions/normL21.jl

src/utilities/normdiff.jl

lostella requested a review from mfalt December 27, 2019 19:31

lostella commented Dec 27, 2019

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

lostella force-pushed the type-stability branch from f4ffbd9 to 2cb5e51 Compare February 14, 2020 22:09

lostella force-pushed the type-stability branch 2 times, most recently from 0d2348a to d522724 Compare April 10, 2020 20:52

lostella and others added 2 commits October 19, 2020 22:27

fix typing issues, refactor tests

c38a0ba

fix issues after rebase

041eea3

lostella force-pushed the type-stability branch from d522724 to 041eea3 Compare October 19, 2020 21:07

Merge branch 'master' into type-stability

5324ad9

mfalt approved these changes Oct 22, 2020

View reviewed changes

lostella added 2 commits November 1, 2020 22:38

simplify enforcing output type

25a1301

fix

3d4a531

lostella requested a review from nantonel November 1, 2020 21:57

nantonel approved these changes Nov 2, 2020

View reviewed changes

lostella merged commit 29d2e3a into JuliaFirstOrder:master Nov 2, 2020

lostella deleted the type-stability branch November 2, 2020 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making function value types the same as input eltype #89

Making function value types the same as input eltype #89

lostella commented Dec 27, 2019 •

edited

codecov bot commented Dec 27, 2019 •

edited

mfalt commented Dec 28, 2019 •

edited

lostella commented Dec 29, 2019

lostella commented Oct 22, 2020

mfalt commented Oct 22, 2020

lostella commented Oct 22, 2020

mfalt commented Oct 28, 2020 •

edited

lostella commented Oct 28, 2020 •

edited

mfalt commented Oct 28, 2020

lostella commented Nov 1, 2020 •

edited

mfalt commented Nov 2, 2020 •

edited

nantonel left a comment •

edited

nantonel Nov 2, 2020

lostella Nov 2, 2020

nantonel Nov 2, 2020

nantonel Nov 2, 2020

Making function value types the same as input eltype #89

Making function value types the same as input eltype #89

Conversation

lostella commented Dec 27, 2019 • edited

codecov bot commented Dec 27, 2019 • edited

Codecov Report

mfalt commented Dec 28, 2019 • edited

lostella commented Dec 29, 2019

lostella commented Oct 22, 2020

mfalt commented Oct 22, 2020

lostella commented Oct 22, 2020

mfalt commented Oct 28, 2020 • edited

lostella commented Oct 28, 2020 • edited

mfalt commented Oct 28, 2020

lostella commented Nov 1, 2020 • edited

mfalt commented Nov 2, 2020 • edited

nantonel left a comment • edited

Choose a reason for hiding this comment

nantonel Nov 2, 2020

Choose a reason for hiding this comment

lostella Nov 2, 2020

Choose a reason for hiding this comment

nantonel Nov 2, 2020

Choose a reason for hiding this comment

nantonel Nov 2, 2020

Choose a reason for hiding this comment

lostella commented Dec 27, 2019 •

edited

codecov bot commented Dec 27, 2019 •

edited

mfalt commented Dec 28, 2019 •

edited

mfalt commented Oct 28, 2020 •

edited

lostella commented Oct 28, 2020 •

edited

lostella commented Nov 1, 2020 •

edited

mfalt commented Nov 2, 2020 •

edited

nantonel left a comment •

edited