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

Sized AbstractArray #783

Merged

Conversation

mateuszbaran
Copy link
Collaborator

Allocation-free statically sized views in Julia 1.5 🎉

The code should be more-or-less complete but I need to add more tests.

One thing that is potentially controversial (and subject to change) is allowing one way of reshaping. Sometimes I represent arrays of different sizes as parts of a long 1-dimensional array, I could of course call reshape before wrapping in SizedArray but that's easy enough to do directly in SizedArray, and currently SizedArray allows for even more reshaping. Relevant PR: #666.

This PR supersedes #341 .

@mateuszbaran mateuszbaran added breaking-change feature features and feature requests labels May 5, 2020
@mateuszbaran mateuszbaran force-pushed the mbaran/sized-abstract-array branch from 6cc24ef to 61c1de9 Compare May 5, 2020 15:26
@mateuszbaran mateuszbaran reopened this May 8, 2020
@c42f
Copy link
Member

c42f commented May 10, 2020

The CI failures here are weird - it's a git error at checkout where supposedly d2adcd0 doesn't exist. But it exists in my local repo.

@mateuszbaran
Copy link
Collaborator Author

I've pushed a new commit and the tests seem to be running OK.

I have one question here: do you think view(::SizedArray, something...) should (where possible) return a SizedArray wrapping a view of the wrapped AbstractArray?

@andyferris
Copy link
Member

I have one question here: do you think view(::SizedArray, something...) should (where possible) return a SizedArray wrapping a view of the wrapped AbstractArray?

To me - the ideal behavior has axes(getindex(a, b)) === axes(view(a, b)).

If this involves effort I would wait until we support axes which are "partially static" (a mixture of OneTo and SOneTo) and we can clean it all up then.

@mateuszbaran
Copy link
Collaborator Author

To me - the ideal behavior has axes(getindex(a, b)) === axes(view(a, b)).

As far as I can tell, this is already the case 🙂 . I'm primarily concerned that without swapping of SubArray and SizedArray when taking a view lots of methods from StaticArrays.jl couldn't be used.

@mateuszbaran mateuszbaran marked this pull request as ready for review June 1, 2020 09:26
@mateuszbaran
Copy link
Collaborator Author

I think this is ready for review now. I have finished views of SizedArray and MArray.

@mateuszbaran mateuszbaran changed the title [WIP] Sized AbstractArray Sized AbstractArray Jun 1, 2020
Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

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

I think this is good-to-go (thanks!) but I'd prefer the MArray view to go into the other file. Julia will still find the new_out_size function.

@mateuszbaran mateuszbaran mentioned this pull request Jul 7, 2020
18 tasks
Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Based on Andy's review I think this should be good to merge.

I went ahead and released 0.12.4 this morning to get that in before this breaking change.

@Liozou Liozou mentioned this pull request Jul 10, 2020
@mateuszbaran mateuszbaran linked an issue Jul 16, 2020 that may be closed by this pull request
@mateuszbaran
Copy link
Collaborator Author

I've fixed one remaining problem with views. Failure on nightly is related to a change in printing of types.

@chriselrod
Copy link
Contributor

Could you define parent now that the parent array's properties determine those of the SizedArray?

@mateuszbaran
Copy link
Collaborator Author

Sure, I've just added parent (and vec).

@mateuszbaran
Copy link
Collaborator Author

Are there any plans to release StaticArrays 0.13 sometime soonish? If not, I think I'll put sized abstract arrays in HybridArrays.jl for now. It would solve many problems with SSubArray.

@c42f
Copy link
Member

c42f commented Sep 16, 2020

@mateuszbaran I approved this, so I was kind of waiting for you to go ahead and merge it. Go on :)

Regarding release, I was imagining we should just release 1.0 next, following the most conservative release plan (ie, just release it more or less as-is not trying to get any more major breaking changes in). IIRC there was one other minor breaking change we should do.

@mateuszbaran
Copy link
Collaborator Author

Great! I will merge it.

Releasing 1.0 would be quite a big thing and I'm happy with your plan about it 👍 .

@mateuszbaran mateuszbaran merged commit 84e0f54 into JuliaArrays:master Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change feature features and feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SizedArray of SubArray isn't a view
4 participants