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

RFC: Rework copy_oftype a bit #44756

Merged
merged 2 commits into from
Mar 30, 2022
Merged

RFC: Rework copy_oftype a bit #44756

merged 2 commits into from
Mar 30, 2022

Conversation

dkarrasch
Copy link
Member

The definition of copy_oftype has been changed in #40831. Subsequently, it was proposed to rename the new function to copymutable_oftype. This PR adds back the original copy_oftype methods and renames the current ones into copymutable_oftype.

The difficulty with this functionality is that it is supposed to do two things at once: (a) copy and (b) potentially change the eltype. If the eltype needs to be changed, it is clear that we need to create a copy. It is not immediatly clear though what type of copy: should some algebraic structure (*diagonality) be preserved, should some wrapped object (think of ranges) be structurally preserved, etc. Even in the case when the eltype doesn't have to be changed, the same questions occur: should the result (or some wrapped structure) be necessarily mutable or not. One value in bringing back the old copy_oftype is that in the latter case (eltype(A) == T) it runs by copy, which is an exported function and may very well be overloaded for custom array types. Generically, though, copy does nothing but copymutable, which in turn is nothing but copyto!(similar(A), A), where similar(A) is generically expanded to similar(A, eltype(A)), and hence copy_oftype does the same as (the new) copymutable_oftype. As for LinearAlgebra internally, I believe we really want those "copies" to be mutable, so we don't use copy_oftype at all. One could argue why have it in LinearAlgebra then, but there was some action required due to the change in the definition of copy_oftype. I'm putting this up mainly for discussion. If this finds support, this should be backportable to v1.8, because this is a complete renaming of any occurence of copy_oftype and returns previously existing methods by their previous name.

@dkarrasch
Copy link
Member Author

Ping @daanhb, @rikhuijzer, @dlfivefifty, @andreasnoack .

@dlfivefifty
Copy link
Contributor

Looks good to me 👍

@daanhb
Copy link
Contributor

daanhb commented Mar 27, 2022

This makes sense to me too. I agree that the name copymutable_oftype is better, since the explicit goal of #40831 was to make sure the result is mutable. Thanks @dkarrasch for revisiting this.

I also see some value in retaining the functionality of copy_oftype for now, for the reasons stated. It certainly has higher chances of being generic and it is no longer a problem that it might return something non-mutable. (It might be useful as a precaution, when returning an array to a user in such a way that the user cannot overwrite internal data later.)

@dkarrasch dkarrasch marked this pull request as ready for review March 28, 2022 11:51
@dkarrasch
Copy link
Member Author

@KristofferC Do you think this is backportable to v1.8? If anything, it should decrease potential tension between LinearAlgebra and downstream packages that use the internal LinearAlgebra.copy_oftype.

@dkarrasch
Copy link
Member Author

Ok, I'll merge and put the backport label on.

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