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

Improve mince and add method for non-uniform partition of IntervalBox #467

Merged
merged 6 commits into from
Jun 3, 2021

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented May 29, 2021

Closes #444

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2021

Codecov Report

Merging #467 (430ed81) into master (65ca77f) will decrease coverage by 0.30%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #467      +/-   ##
==========================================
- Coverage   91.53%   91.22%   -0.31%     
==========================================
  Files          25       25              
  Lines        1748     1812      +64     
==========================================
+ Hits         1600     1653      +53     
- Misses        148      159      +11     
Impacted Files Coverage Δ
src/intervals/arithmetic.jl 97.20% <100.00%> (ø)
src/multidim/intervalbox.jl 88.88% <100.00%> (-0.59%) ⬇️
src/intervals/conversion.jl 55.81% <0.00%> (-13.96%) ⬇️
src/intervals/rounding.jl 61.36% <0.00%> (-5.69%) ⬇️
src/intervals/macros.jl 100.00% <0.00%> (ø)
src/intervals/special.jl 100.00% <0.00%> (ø)
src/intervals/intervals.jl 94.62% <0.00%> (+3.09%) ⬆️

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 65ca77f...430ed81. Read the comment docs.

@lbenet
Copy link
Member Author

lbenet commented May 29, 2021

Some benchmarking:

  • Current master:
julia> @benchmark mince(ii, ndiv) setup=(ii=-1 .. 1, ndiv=100_000_000)
BenchmarkTools.Trial: 
  memory estimate:  152.59 MiB
  allocs estimate:  2
  --------------
  minimum time:     45.498 ms (0.00% GC)
  median time:      64.020 ms (26.33% GC)
  mean time:        58.819 ms (17.82% GC)
  maximum time:     120.155 ms (58.98% GC)
  --------------
  samples:          85
  evals/sample:     1
  • This PR
julia> @benchmark mince(ii, ndiv) setup=(ii=-1 .. 1, ndiv=100_000_000)
BenchmarkTools.Trial: 
  memory estimate:  152.59 MiB
  allocs estimate:  2
  --------------
  minimum time:     41.535 ms (0.00% GC)
  median time:      53.344 ms (16.84% GC)
  mean time:        54.540 ms (19.06% GC)
  maximum time:     117.183 ms (60.92% GC)
  --------------
  samples:          92
  evals/sample:     1

src/intervals/arithmetic.jl Outdated Show resolved Hide resolved
src/multidim/intervalbox.jl Outdated Show resolved Hide resolved
end

nodes = IntervalBox{$N,T}[]
Base.Cartesian.@nloops $N i _->(1:n) begin
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this part requires carefull review? I am not familiar with the macros used, but I could delve into it if needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review @Kolaru and also for offering getting into the details of this part of the code.

Here, I simply used the original method, and adapt it to obtain the desired behavior; maybe there are better ways, but for the time being, this is good enough. The original code was inspired from this section of the manual, which admittedly I have mostly forgotten.

Copy link
Collaborator

@Kolaru Kolaru Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounded like a fun multidimensional challenge so I spent some time on it and I believe the following is equivalent:

function mince(x::IntervalBox{N,T}, ncuts::NTuple{N,Int}) where {N,T}
    minced_intervals = [mince(x[i], ncuts[i]) for i in 1:N]
    minced_boxes = Vector{IntervalBox{N,T}}(undef, prod(ncuts))

    for (k, cut_indices) in enumerate(CartesianIndices(ncuts))
        minced_boxes[k] = IntervalBox([minced_intervals[i][cut_indices[i]] for i in 1:N])
    end
    return minced_boxes
end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Kolaru for the suggestion! I am also changing the other method (avoiding then the @generated). I would like to include you as coauthor of this commit; is it ok with you? (I already have the commit ready, but prefer to wait for your reply before pushing it.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure let me know if it needs any input from my side.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed it, so you can have a look

lbenet and others added 2 commits May 31, 2021 17:52
Co-authored-by: Benoît Richard <kolaru@hotmail.com>
@Kolaru Kolaru merged commit a3dc27f into master Jun 3, 2021
@Kolaru
Copy link
Collaborator

Kolaru commented Jun 3, 2021

All look good, thanks.

@lbenet
Copy link
Member Author

lbenet commented Jun 3, 2021

Thanks a lot!

@lucaferranti lucaferranti deleted the lb/mince branch January 25, 2022 21:08
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.

Generalize IntervalBox split for non-uniform partitions
3 participants