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

implement faster issubset for CartesianIndices{N} #56282

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

xili-h
Copy link
Contributor

@xili-h xili-h commented Oct 22, 2024

No description provided.

@Seelengrab
Copy link
Contributor

Do you have some benchmarks showing a performance improvement?

@xili-h
Copy link
Contributor Author

xili-h commented Oct 22, 2024

Do you have some benchmarks showing a performance improvement?

image

@xili-h
Copy link
Contributor Author

xili-h commented Oct 22, 2024

For 1D, it is also faster.
image
image

@LilithHafner
Copy link
Member

Thanks for looking into this, @xili-h! This is a good specialization to have.

This clearly has higher performance than the generic fallback, especially because the indices themselves have O(1) subset checking.

However, there's a correctness problem: if the first argument is empty, then it is always a subset, even if some of its indices are not subsets because there are no elements to witness that axis being larger:

julia> a = CartesianIndices((0, 1))
CartesianIndices((0, 1))

julia> b = CartesianIndices((1,0))
CartesianIndices((1, 0))

julia> issubset(a, b)
true

julia> _issubset(a::CartesianIndices{N}, b::CartesianIndices{N}) where N =
           all(map(issubset, a.indices, b.indices))
_issubset (generic function with 2 methods)

julia> _issubset(a,b)
false

julia> _issubset2(a::CartesianIndices{N}, b::CartesianIndices{N}) where N =
           isempty(a) || all(map(issubset, a.indices, b.indices))
_issubset2 (generic function with 1 method)

julia> _issubset2(a,b)
true

As an aside, I prefer if you paste results as text rather than screenshots because that makes it easier for folks to copy your code and reproduce it locally.

@xili-h
Copy link
Contributor Author

xili-h commented Oct 22, 2024

Thanks for looking into this, @xili-h! This is a good specialization to have.

This clearly has higher performance than the generic fallback, especially because the indices themselves have O(1) subset checking.

However, there's a correctness problem: if the first argument is empty, then it is always a subset, even if some of its indices are not subsets because there are no elements to witness that axis being larger:

julia> a = CartesianIndices((0, 1))
CartesianIndices((0, 1))

julia> b = CartesianIndices((1,0))
CartesianIndices((1, 0))

julia> issubset(a, b)
true

julia> _issubset(a::CartesianIndices{N}, b::CartesianIndices{N}) where N =
           all(map(issubset, a.indices, b.indices))
_issubset (generic function with 2 methods)

julia> _issubset(a,b)
false

julia> _issubset2(a::CartesianIndices{N}, b::CartesianIndices{N}) where N =
           isempty(a) || all(map(issubset, a.indices, b.indices))
_issubset2 (generic function with 1 method)

julia> _issubset2(a,b)
true

As an aside, I prefer if you paste results as text rather than screenshots because that makes it easier for folks to copy your code and reproduce it locally.

Thanks @LilithHafner. I have fix the problem base on your suggestion. I will paste results as text next time.

@nsajko nsajko added the collections Data structures holding multiple items, e.g. sets label Oct 22, 2024
@LilithHafner LilithHafner added the performance Must go faster label Oct 23, 2024
@LilithHafner LilithHafner merged commit 0b9fcb5 into JuliaLang:master Oct 23, 2024
6 of 9 checks passed
@LilithHafner
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants