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

add dims keyword for padded_views and sym_paddedviews #49

Merged
merged 8 commits into from
Aug 5, 2021

Conversation

ashwanirathee
Copy link
Contributor

@ashwanirathee ashwanirathee commented Mar 15, 2021

@johnnychen94 Initial PR on addition of keyword dims in padded views and also in sym_padded_views later

closes #48

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Generally looks good, except that the current implementation is 2D specific; we actually want to support arbitrary dimensions.

I suggest that you write down some tests in test/runtest.jl and test it via

julia --project=. -e "using Pkg; Pkg.test()"

This helps you become confident in your implementation.

Also, when the functionality is done, don't forget to add documentation, and update README.md.

Comment on lines 222 to 224
dims1 = 1
dims2 = (1,2)
dims3 = 1:2
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, PaddedView handles arrays of arbitrary dimensions, for example, As can be a list of arrays with dimension N=4. I don't think the current code handles N>2 case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnnychen94 Can the person provide the tuple in like in any random fashion..like can I provide it in following ways:

  • (1,2,4,5,3) - I need sort and unique for this
  • (1,2,3,4,5)- if it's continous and properly sorted,then it won't be a issue
  • (1,1,1) - also this case
  • (0,1,2,...) - also this case

Copy link
Member

@johnnychen94 johnnychen94 Mar 15, 2021

Choose a reason for hiding this comment

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

I'll follow the cat convention:

julia> cat([1,2], [3 4]; dims=(2,1))
3×3 Array{Int64,2}:
 1  0  0
 2  0  0
 0  3  4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we get dimension mismatch issue when arrays data is returned,which is mosaicviews data handling issue but that does arise for some reason I don't get yet...though we won't be talking about it here,right?

Copy link
Member

Choose a reason for hiding this comment

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

The dimension mismatch issue you mentioned in #48 is not very clear to me, they should be unrelated. My best guess is that once we consolidate this feature in the PaddedViews side, the issue got fixed automatically.

FYI, there's a PR (JuliaArrays/MosaicViews.jl#31) related to the _padded_cat you're referring to, so no, I'm not going to check it until we get the current PaddedViews PR done.

src/PaddedViews.jl Outdated Show resolved Hide resolved
@ashwanirathee
Copy link
Contributor Author

ashwanirathee commented Mar 23, 2021

Maybe the JuliaFormatter messed it all up a little,I formatted the whole code and now we have conflicts 😓 .I formatted it and it itself formatted too the code in vs code whenever save button is pushed.

@johnnychen94
Copy link
Member

johnnychen94 commented Mar 23, 2021

Yeah... could you revert the unrelated changes? It's a little bit mess to review the diffs now...

@ashwanirathee
Copy link
Contributor Author

I think this does the job for the paddedviews, next is sym_ paddedviews

@johnnychen94
Copy link
Member

Didn't check very carefully, but it looks good to me so far. Will recheck it when sym_paddedviews is supported.

@ashwanirathee
Copy link
Contributor Author

without collect, it throws errors, Needs to check why

test/runtests.jl Outdated
a1f, a2f = sym_paddedviews(-1, a1, a2;dims=(1))
@test a2f == a2c
a1f, a2f = sym_paddedviews(-1, a1, a2;dims=(2))
@test collect(a2f) == a2r
Copy link
Member

Choose a reason for hiding this comment

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

without collect, it throws errors, Needs to check why

PaddedArray might not be 1-based Array, to properly compare two arrays, you need to make sure their axes are the same.

# from `@less ==(rand(4, 4), rand(4, 4))
function (==)(A::AbstractArray, B::AbstractArray)
    if axes(A) != axes(B)
        return false
    end
    anymissing = false
    for (a, b) in zip(A, B)
        eq = (a == b)
        if ismissing(eq)
            anymissing = true
        elseif !eq
            return false
        end
    end
    return anymissing ? missing : true
end

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

LGTM, can you also update the docstrings and README examples? When that's done I think it's okay to merge this.

@johnnychen94 johnnychen94 changed the title Addition of Dims Keyword add dims keyword for padded_views and sym_paddedviews Aug 5, 2021
@johnnychen94 johnnychen94 merged commit d6f4474 into JuliaArrays:master Aug 5, 2021
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.

Dims Keyword addition
2 participants