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 unwrap function to allow wrapper types to avoid over-overloading convert #32613

Closed
wants to merge 2 commits into from

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Jul 17, 2019

Would allow packages like CategoricalArrays and DataValues to avoid having to overload convert(::Type{Any}, x) with their own definitions (due to wanting generic convert(::Type{T}, x::MyType) definitions). Just putting this up as a POC to get feedback.

Ref: JuliaData/CategoricalArrays.jl#177, queryverse/DataValues.jl#51


function convert(::Type{T}, x) where {T}
y = unwrap(x)
return y === x ? x : convert(T, y)
Copy link
Member

Choose a reason for hiding this comment

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

MethodError if y === x?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wasn't quite sure on how this should interact (replace?) convert(Any, x), so I just simulated convert to Any

Copy link
Member

Choose a reason for hiding this comment

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

But convert(T, x) is supposed to return something of type T. Returning x works for Any but not all T.

@bkamins
Copy link
Member

bkamins commented Jul 25, 2019

Just a report of possible consequence of not having this (this is on Windows, Julia 1.1, on Linux and OSx the problem does not seem to be that big - which is surprising):

$ julia -e "@time using Missings, CategoricalArrays"
  0.712632 seconds (2.05 M allocations: 137.079 MiB, 2.84% gc time)

$ julia -e "@time using CategoricalArrays, Missings"
  2.155997 seconds (5.49 M allocations: 299.823 MiB, 4.72% gc time)

CC @nalimilan

@bkamins
Copy link
Member

bkamins commented Jul 25, 2019

The same timings for Linux:

$ julia-1.1.1/bin/julia -e "@time using Missings, CategoricalArrays"
  0.615162 seconds (2.02 M allocations: 136.352 MiB, 2.88% gc time)
$ julia-1.1.1/bin/julia -e "@time using CategoricalArrays, Missings"
  1.811056 seconds (5.19 M allocations: 286.239 MiB, 5.31% gc time)

@quinnj quinnj marked this pull request as ready for review July 26, 2019 13:50
@quinnj
Copy link
Member Author

quinnj commented Jul 26, 2019

Alright, added the MethodError, docs for Base.unwrap, and some tests (showing usefulness)

"""
Base.unwrap(x)

Lower, or "unwrap" a value that has Base-defined representation. Currently
Copy link
Member

Choose a reason for hiding this comment

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

What do "lower" and "Base-defined" mean exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Base-defined really means that you convert/transform your wrapper type to a value with a type that already has a convert(T, x) definition, which for the most part, will be Base/stdlib-defined types.

Copy link
Member

Choose a reason for hiding this comment

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

OK. That doesn't sound very explicit to me, and if that's only try "for the most part" it shouldn't be presented as a general property. Why not just say "unwrap"?

wrapperstruct = WrapperStruct("hey")
@test_throws MethodError convert(String, wrapperstruct)
Base.unwrap(x::WrapperStruct) = x.x
@test convert(String, wrapperstruct) == "hey"
Copy link
Member

Choose a reason for hiding this comment

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

Also test that it works when the destination type differs from typeof(unwrap(wrapperstruct))? E.g. with Int and Float64.

@quinnj
Copy link
Member Author

quinnj commented Jul 29, 2019

@JeffBezanson, this is ready for another review when you have a moment.

@davidanthoff
Copy link
Contributor

Do we really need this? I just retested queryverse/DataValues.jl#52, which simply removes that offending convert method, and things work on julia 1.1 and 1.2 (but break on 1.0). Maybe that is good enough, I could put that method behind a VERSION check?

I'm not keen of thinking of DataValue as a wrapper, I'd much prefer to simply think of it as a type that supports the values from some other type plus one extra value, but not pin down any internal way to represent that or a mental idea of a wrapper. Would the idea here be that unwrap is a more generic API than just to solve this recompile issue, i.e. some way to introduce a generic idea of wrapper types into julia? If so, at least for DataValue, I think I would like to think through what that actually means, implies before I would commit to it. If the goal here is only to solve the recompile issue, though, then unwrap seems too generic of a name to me that seems to carry quite a lot of implications of generic API design with it.

@quinnj
Copy link
Member Author

quinnj commented Aug 5, 2019

No, this isn't meant as a generic "wrapper" API or anything; unwrap isn't exported from Base and is just meant for this convert scenario (and could be renamed to something else if something like a "wrapper" API became a thing). I'll look into the PR that removes the method a little more; I'm interested to see how it works and what we'd need to do in CategoricalArrays similarly.

@davidanthoff
Copy link
Contributor

and could be renamed to something else if something like a "wrapper" API became a thing

In that case, why not use a less "good" name now and preserve unwrap for some future use in Base? Yes, one could rename, but that would trigger churn in downstream packages that we could easily avoid by picking some other name now.

@quinnj
Copy link
Member Author

quinnj commented Jul 20, 2020

I don't think we're going with this approach, as it's a bit of a hack anyway. I'm beginning to believe that might not be reasonable for CategoricalValue to expect to act like "any" value it wraps; I think moving to an explicit wrap/unwrap API would alleviate a lot of its problems.

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.

5 participants