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

Force specialization on mutable operate #116

Merged
merged 1 commit into from
Oct 12, 2021
Merged

Force specialization on mutable operate #116

merged 1 commit into from
Oct 12, 2021

Conversation

blegat
Copy link
Member

@blegat blegat commented Oct 11, 2021

@kalmarek Can you confirm that it fixes your issue ?

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #116 (57b2f6c) into master (3f455a4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #116   +/-   ##
=======================================
  Coverage   85.29%   85.29%           
=======================================
  Files          18       18           
  Lines        1673     1673           
=======================================
  Hits         1427     1427           
  Misses        246      246           
Impacted Files Coverage Δ
src/interface.jl 93.96% <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 3f455a4...57b2f6c. Read the comment docs.

@kalmarek
Copy link

not really:

activate --temp
add MutableArithmetics#bl/mutable_op_F
add JuMP, Random, LinearAlgebra
using LinearAlgebra, JuMP, Random
let dims = [31, 24, 49, 30, 33, 49, 33, 21, 44, 50, 50, 32, 31, 48, 28, 34, 35, 41, 25, 43]
    model = Model()
    Ps = [
        JuMP.@variable(model, [1:d, 1:d])
        for d in dims
    ]
    Ms = [rand(d,d) for d in dims]
    
    @time JuMP.@constraint(model, sum(dot(P, M) for (P,M) in zip(Ps, Ms)) == 1.0 )
    @time JuMP.@constraint(model, sum(dot(Ps[i], Ms[i]) for i in eachindex(dims)) == 1.0 )
    
    model
end;

gives (after warmup)

0.007942 seconds (208.41 k allocations: 9.852 MiB)
0.009449 seconds (208.33 k allocations: 9.849 MiB)

@blegat
Copy link
Member Author

blegat commented Oct 12, 2021

Comparing to the timings of #111, it looks like the timings of JuMP v0.20

@kalmarek
Copy link

kalmarek commented Oct 12, 2021

Indeed time spent is as of JuMP v20 ;) allocations are not altered by this change though

(jl_1XzHFh) pkg> st
      Status `/tmp/jl_1XzHFh/Project.toml`
  [4076af6c] JuMP v0.21.10
  [d8a4904e] MutableArithmetics v0.2.21 `https://github.com/jump-dev/MutableArithmetics.jl.git#bl/mutable_op_F`
  [37e2e46d] LinearAlgebra
  [9a3f8284] Random

EDIT: well actually the number of allocations has doubled, but not necessarily because of this change ;)

@blegat blegat merged commit 1bdf2b6 into master Oct 12, 2021
@blegat
Copy link
Member Author

blegat commented Oct 12, 2021

As mentioned in #111 (comment), I don't know what's happening with allocations :/

@odow odow deleted the bl/mutable_op_F branch October 17, 2021 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants