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

wrap is a very generic name to export #53552

Closed
jariji opened this issue Mar 2, 2024 · 7 comments · Fixed by #53896
Closed

wrap is a very generic name to export #53552

jariji opened this issue Mar 2, 2024 · 7 comments · Fixed by #53896
Milestone

Comments

@jariji
Copy link
Contributor

jariji commented Mar 2, 2024

wrap was introduced as the safe alternative to unsafe_wrap in #52049 in December by @MasonProtter.

Maybe this is the right call but I want to at least flag the issue. wrap a very a generic name and there are many packages using it for all kinds of purposes, and many others that will likely want to choose that name in the future. 288 files on GitHub define wrap, with some duplicates.

Options:

  • No change. Users will qualify Package.wrap instead of using packages. Many people who would write a function wrap will probably decide to choose a different name for their function. Heavy users of Base.wrap will be pleased.
  • Don't export wrap. Users will write Base.wrap instead of wrap, which could be annoying if it's in common usage, but will be more pleasant for authors and users of other functions named wrap.
  • Rename it. wrap_memory? Array? array? memoryarray? something else?
@MasonProtter
Copy link
Contributor

MasonProtter commented Mar 2, 2024

I feel like I remember some reason why we didn't want to do Array, but now I can't remember what it was. Array seems like an okay name for it though

I also think though that wrap it's a fine name for most packages to add methods to, since the base method is just concerned specifically with a restricted type signature, but the overall pattern is very generalizable, since it's just

wrap(the_type, the_object, [the_size])

I guess I also just like the symmetry because it really is basically the same thing as unsafe_wrap except it's safe.

@LilithHafner
Copy link
Member

Most of the definitions of wrap in the linked results do not follow the wrap(the_type, the_object, [the_size]) pattern and many are not compatible with it. I think this is a good candidate for public but not exported.

@LilithHafner LilithHafner added the triage This should be discussed on a triage call label Mar 28, 2024
@LilithHafner
Copy link
Member

Adding triage label per @jariji's request in the #triage slack channel

@LilithHafner LilithHafner added this to the 1.11 milestone Mar 28, 2024
@LilithHafner
Copy link
Member

From triage, for 1.11, we should delete wrap (replace current usages with view)

view(::Memory, inds) should return an Array. (currently returns a view object)
reshape(::Memory, dims) should also return an Array. (currently returns a reshape object)

View supports preallocating for an empty Vector view(Memory{T}(undef, 5), 1:0)
reshape supports everything else.


Other options we didn't like

  • Array(::Memory, dims) is not great because Array(::AbstractArray) and Array(::Memory) copy (though Array(::AbstractArray, dims) does not exist yet). "Adding dims => it doesn't copy" is a weird mnemonic.
  • Array! but it doesn't actually mutate its arguments.

@LilithHafner LilithHafner removed the triage This should be discussed on a triage call label Mar 28, 2024
@LilithHafner
Copy link
Member

See also LinearAlgebra.wrap for additional evidence that this name is widely used with incompatible APIs.

LilithHafner added a commit that referenced this issue Apr 6, 2024
- Make reshape and view with one based indexing on Memory produce Arrays
- delete wrap

Implements
#53552 (comment)

---------

Co-authored-by: Jameson Nash <[email protected]>
@Seelengrab
Copy link
Contributor

For posteriority, what was the reasoning for having view on Memory return an Array? Seems a bit counterintuitive, considering that the "view" can then grow and the underlying bulk-storage can't, while so far it's been the other way around.

@oscardssmith
Copy link
Member

The primary reasoning is that we somewhat bacronymmed this. The thing we wanted was a way to construct an Array from a memory with specific bounds, and we realized that view has the right semantics.

KristofferC pushed a commit that referenced this issue Apr 9, 2024
- Make reshape and view with one based indexing on Memory produce Arrays
- delete wrap

Implements
#53552 (comment)

---------

Co-authored-by: Jameson Nash <[email protected]>
(cherry picked from commit 273d91e)
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 a pull request may close this issue.

5 participants