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 three-argument similar methods #94

Closed
wants to merge 2 commits into from

Conversation

Keno
Copy link
Contributor

@Keno Keno commented Oct 15, 2019

Array wrappers such as OffsetArray assume that the three argument
version exists for its interior arrays.

Also cc @mbauman for array expertise.

Array wrappers such as `OffsetArray` assume that the three argument
version exists for its interior arrays.
@codecov-io
Copy link

codecov-io commented Oct 15, 2019

Codecov Report

Merging #94 into master will decrease coverage by 0.73%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
- Coverage   91.03%   90.29%   -0.74%     
==========================================
  Files           9        9              
  Lines         368      371       +3     
==========================================
  Hits          335      335              
- Misses         33       36       +3
Impacted Files Coverage Δ
src/structarray.jl 84.84% <0%> (-1.75%) ⬇️
src/collect.jl 93.65% <0%> (-1.52%) ⬇️

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 7d399b1...04105c9. Read the comment docs.

@@ -102,10 +102,18 @@ end

Base.similar(s::StructArray, sz::Base.DimOrInd...) = similar(s, Base.to_shape(sz))
Base.similar(s::StructArray) = similar(s, Base.to_shape(axes(s)))
function Base.similar(s::StructArray{T}, sz::Tuple) where {T}
function Base.similar(s::StructArray{T,N,C}, ::Type{T}, sz::NTuple{N,Int64}) where {T, N, C<:Union{Tuple, NamedTuple}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this too strict? I'm trying to wrap my head around whether the eltype and the passed type should really match or if it's enough that one is a subtype of the other.

@piever
Copy link
Collaborator

piever commented Oct 16, 2019

Thanks! I think I hadn't implemented this to avoid the issue you mention in the comment (one has to guess the array type as fields may not match), but am happy to merge this change if it helps with OffsetArrays.

src/structarray.jl Outdated Show resolved Hide resolved
src/structarray.jl Outdated Show resolved Hide resolved
src/structarray.jl Outdated Show resolved Hide resolved
@lcw
Copy link
Contributor

lcw commented Apr 2, 2021

I find the three-argument similar methods useful and I hope these changes will get merged.

Co-authored-by: Lucas C Wilcox <lucas@swirlee.com>
piever added a commit that referenced this pull request Jun 17, 2022
* Implement three-argument `similar` methods

Array wrappers such as `OffsetArray` assume that the three argument
version exists for its interior arrays.

* Apply suggestions from code review

Co-authored-by: Lucas C Wilcox <lucas@swirlee.com>

* simplify similar signatures

* support and test similar and reshape with offsets

Co-authored-by: Keno Fischer <keno@alumni.harvard.edu>
Co-authored-by: Lucas C Wilcox <lucas@swirlee.com>
@piever
Copy link
Collaborator

piever commented Jun 17, 2022

Merged in #218

@piever piever closed this Jun 17, 2022
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.

4 participants