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 Future.copy! function #25144

Merged
merged 1 commit into from
Dec 29, 2017
Merged

add Future.copy! function #25144

merged 1 commit into from
Dec 29, 2017

Conversation

rfourquet
Copy link
Member

This implements the suggestion proposed in #24808 to have Future.copy! available in Base right now, which will be moved to Base for 1.0. Unfortunately, the name Future is already taken, so I temporarily named it Next till a decision is taken for the name.

I implemented this new version of copy! in a conservative way: copy!(a, b) works only if a and b are in the same abstract hierachy, i.e arrays with arrays, sets with sets etc. We could be even more strict by requiring a and b to be of the same type, but that doesn't seem useful. One caveat is that we don't necessarily have a==b after the call, because for example [1, 2] != 1:2 or Set(1) != BitSet(1). But my understanding is that we would like to have these examples be equal.

A less conservative approach would be to allow copying any iterable into the destination, but I feel this is much more debatable, as in this case there is no way we will have equality of a and b after the call, e.g. in copy!(rand(2, 3), rand(3, 2)), the 2 arguments don't have the same shape. Or in copy!(Set(1), rand(2, 3)) we don't even have the same kind of collection. My feeling is that this use-case is kind of in between copy! and copyto!, so I would prefer to not rush with this one and take the time to think about it. In the meantime, it's seems enough to suggest simple alternatives, like union!(empty!(Set(1)), rand(2, 3)), or copyto!(rand(2, 3), rand(3, 2)).

@rfourquet rfourquet added the collections Data structures holding multiple items, e.g. sets label Dec 17, 2017
@rfourquet rfourquet added this to the 1.0 milestone Dec 17, 2017
base/future.jl Outdated

function copy!(dst::AbstractArray, src::AbstractArray)
size(dst) == size(src) || throw(ArgumentError(
"arrays must have the same dimension for copy! (condiser using `copyto!`)"))
Copy link
Contributor

@GunnarFarneback GunnarFarneback Dec 17, 2017

Choose a reason for hiding this comment

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

Misspelling of consider. And isn't it rather "same size" than "same dimension"?

@fredrikekre
Copy link
Member

FWIW I like Future better than Next, you seem to have used both names in this PR.

@StefanKarpinski
Copy link
Member

I like Future better as well (and it has precedent from Python).

@rfourquet
Copy link
Member Author

I also definitely prefer Future, but as I said it's "taken" (now it's @deprecate_moved into "Distributed"), so the compilation fails with Future instead of Next.

@rfourquet
Copy link
Member Author

So I hadn't thought about it earlier, but making Future an stdlib package instead of a Base submodule doesn't seem to create conflict with the current Base.Future deprecated name. If CI passes, it would be good to go I believe.

@rfourquet
Copy link
Member Author

I will merge some day next week, if no objection and no one beats me to it.

@rfourquet rfourquet changed the title [WIP] add Future.copy! function add Future.copy! function Dec 24, 2017

function copy!(dst::AbstractArray, src::AbstractArray)
size(dst) == size(src) || throw(ArgumentError(
"arrays must have the same dimension for copy! (condiser using `copyto!`)"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider is still misspelled.

@rfourquet rfourquet merged commit d0ed2f2 into master Dec 29, 2017
@rfourquet rfourquet deleted the rf/future-copybang branch December 29, 2017 11:54
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants