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

make coalesce handle only missing; add something #27258

Merged
merged 2 commits into from
May 30, 2018
Merged

Conversation

JeffBezanson
Copy link
Sponsor Member

This is a possible approach to fixing #26927. Overall I like it. The name something is ok, but didn't feel perfect. Basically all uses of coalesce in Base were for providing default values for find or match functions, and something(findnext(...), 0) doesn't read quite right. I also notice that it's very similar to notnothing, and the functions could perhaps be combined (one way or the other).

@JeffBezanson JeffBezanson added the domain:missing data Base.missing and related functionality label May 25, 2018
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Thanks! Looks reasonable to me.

julia> coalesce(1, missing)
1

julia> coalesce(nothing, 1) # returns `nothing`
Copy link
Member

Choose a reason for hiding this comment

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

Probably better remove this example, there's no reason to think that nothing is treated differently from any other value.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think the point of the example is to demonstrate that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I understand that, but my point is that nobody would have imagined coalesce would consider nothing as a missing value, except for the fact that it was the previous behavior. And since most people won't know that history...

Anyway, not a big deal.

"""
notnothing(x::Any) = x
notnothing(::Nothing) = throw(ArgumentError("nothing passed to notnothing"))
something() = throw(ArgumentError("No value arguments present"))
Copy link
Member

@nalimilan nalimilan May 26, 2018

Choose a reason for hiding this comment

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

Same remark as for the zero-argument coalesce below.

@@ -33,47 +33,39 @@ function show(io::IO, x::Some)
end

"""
coalesce(x, y...)
notnothing(x)
Copy link
Member

Choose a reason for hiding this comment

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

As you noted, I think we can just remove notnothing and use something instead. I don't think we use it in situations where accidentally unwrapping Some is possible. On the contrary, I only used notnothing in places where Nullable previously required explicit unwrapping, which ensured null values were caught early. Anyway it's internal.

@@ -228,6 +228,7 @@ Base.missing
Base.coalesce
Base.ismissing
Base.skipmissing
Base.something
Copy link
Member

Choose a reason for hiding this comment

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

Should be listed with nothing and Some instead, it's totally unrelated to missing.

"""
function coalesce end

coalesce() = missing
Copy link
Member

Choose a reason for hiding this comment

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

I'd find it more natural to define coalesce(x::Missing) = throw(...). There's no reason to define a function with zero arguments AFAICT. Same remark for something.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

This isn't ancient Rome; we understand zero now 😁

Copy link
Sponsor Member

@StefanKarpinski StefanKarpinski May 26, 2018

Choose a reason for hiding this comment

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

I 100% agree with Jeff here. Zero things is missing the same way the sum of no things is zero and the product of no things is one. If we knew the type in those cases, we would absolutely define the empty sums and products to do that; in this case we do know the type and can return missing.

Copy link
Member

@nalimilan nalimilan May 26, 2018

Choose a reason for hiding this comment

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

OK, why not, though I'm not sure in what cases it could be useful. OTOH I don't think it makes sense for something (since an error is thrown), but let's discuss it in the other thread above. EDIT: Actually now I see it's somewhat similar, even though it's not that useful since an error is thrown anyway.

@nalimilan
Copy link
Member

I also realized it would be nice to have a deprecation for coalesce with nothing, given that the deprecations for search & find methods have recommended using coalesce. Without it, people who have ported their packages early will be left with broken code without any indication about what to do.

@nalimilan
Copy link
Member

I've rebased and added a commit moving the docs. I'm not sure how to correctly add a deprecation, given that the behavior has changed in an incompatible way. We could just print a warning pointing to something when coalesce is passed nothing or Some.

@ViralBShah
Copy link
Member

Why did freebsd not happen here?

@ViralBShah
Copy link
Member

This PR was all green but the conflicts are out of merging the statistics-stdlib PR. I think we should try and merge it today, if the PR is otherwise ready.

@ViralBShah
Copy link
Member

Rebased on master. Might as well let CI have another go at it.

@iblis17 FreeBSD doesn't seem to be picking this up.

@iblislin
Copy link
Member

@ViralBShah I think it built already. just queuing before.

@ViralBShah
Copy link
Member

Thanks @iblis17. So all green! Is something the name that everyone wants and settled?

@JeffBezanson
Copy link
Sponsor Member Author

I think I might prefer notnothing --- it's very clear, since this returns the first value that's not nothing.

@JeffBezanson
Copy link
Sponsor Member Author

I believe F# calls this defaultArg. I also find that appealing, since this can be described as supplying a default value when a certain value is absent. Maybe defaultval.

@JeffBezanson
Copy link
Sponsor Member Author

Another data point: fromMaybe in Haskell.

@nalimilan
Copy link
Member

nalimilan commented May 28, 2018

FWIW, Scala uses orElse. EDIT: and Rust uses unwrap (single argument) and unwrap_or (two arguments).

something and notnothing sound more explicit than other choices to me. Maybe let's have a quick poll?

@JeffBezanson
Copy link
Sponsor Member Author

Scala also has getOrElse. Kind of surprising how little agreement there is on what this function is called.

@nalimilan
Copy link
Member

Yeah. The terminology "null-coalescing operator" is more standard, but even though ?? is the most common there's also a lot of variation.

@JeffBezanson JeffBezanson added this to the 0.7 milestone May 28, 2018
@JeffBezanson
Copy link
Sponsor Member Author

How about we go with orelse? It's short, and findfirst(...) orelse 0 reads well.

@quinnj
Copy link
Member

quinnj commented May 30, 2018

I like orelse

@nalimilan
Copy link
Member

I'm not a big fan of orelse. It doesn't read very well in the function form orelse(x, y). Also if we want an infix operator, why not use ???

@ViralBShah
Copy link
Member

I suggest merging this as is for now, in order to not block the alpha, and changing the name to something else should we want to before the release candidate. something seems better than coalesce to me, and about as good as many of the names suggested here.

@nalimilan
Copy link
Member

Personally, I'm fine with something, but I think we'd better make a hard decision on the API before tagging the alpha. Else people are going to port their packages to the new API (which is used in many places due to to the find & search deprecations), only to find out they need to change it again for the beta.

@ViralBShah
Copy link
Member

I am going to merge this now. We still have the rest of the day or maybe even tomorrow to pick a different name.

@yurivish
Copy link
Contributor

yurivish commented May 30, 2018

I like ??.

@ViralBShah ViralBShah merged commit 8aa54ac into master May 30, 2018
@martinholters martinholters deleted the jb/missingsomething branch May 30, 2018 18:50
@StefanKarpinski
Copy link
Sponsor Member

After some initial skepticism, I've come to like ?? for this operation but that seems to push very hard in the direction of using ? for nothingness rather than missingness, which would likely include T? as a syntax for Union{T, Nothing} as opposed to Union{T, Missing}.

@KristofferC
Copy link
Sponsor Member

which would likely include T? as a syntax for Union{T, Nothing} as opposed to Union{T, Missing}.

Totally think that is a better choice.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented May 30, 2018

That would dovetail with a whole other suite of ? syntaxes that I think are pretty useful:

  • T? for Union{T, Nothing}
  • x ?? default for something(x, default)
  • x ?= default for x = something(x, default)
  • d[k] ?? default for get(()->default, d, k)
  • d[k] ?= default for get!(()->default, d, k)

The collection of them is fairly compelling since they're all nothingesque rather than missingesque.

@StefanKarpinski
Copy link
Sponsor Member

That actually suggests an unexpected option that doesn't involve new syntax: get(x, default).

@JeffBezanson
Copy link
Sponsor Member Author

I assume we would want ?? to evaluate its second argument lazily (in all cases)? If so, there would still be a need for a function version of it. I like get; I thought there might already be 1- or 2-argument methods of it but that doesn't seem to be the case. Also has a slight precedent in Scala's getOrElse. Its conciseness can't be beat and it is definitely similar to get on dicts.

I also have to admit this makes me entertain the heretical thought that we should consider combining nothing and missing. Even though they have a clear difference in meaning, the extra simplicity and syntax possibilities might be worth it.

@StefanKarpinski
Copy link
Sponsor Member

Yes, it would be lazy, of course, I should have indicated that. Having both nothing and missing has turned out to be more annoying than I had anticipated but it's a bit late in the game to change now.

@JeffBezanson
Copy link
Sponsor Member Author

True. The ? proposals could give us a clear division of labor: nothing gets various ? syntaxes, and missing gets 3VL and some common functions defined on it. Then we get to see if one ends up being much more useful, or if both are equally useful in different contexts.

@nalimilan
Copy link
Member

nalimilan commented May 30, 2018

It would totally make sense to use all the ? syntaxes for nothing. missing doesn't need them, it just needs another syntax akin to T?.

EDIT: We used get to unwrap Nullable, but it didn't really turn out to be a good idea IMHO. It's a quite different operation from getting a dict entry, where the second argument is the key. The only similarity is the presence of a default value.

@JeffBezanson
Copy link
Sponsor Member Author

My impression is that the main problem with get on Nullable was that people didn't want to unwrap the values at all, not that get was a bad name for it. Also, you can imagine generalizing get to multiple indices: get(dict, idxs..., default), where get(dict, default) is the 0-index case. Some would be acting like a 0-d container.

@nalimilan
Copy link
Member

But then it's weird when not all arguments are Some, and some calls like get("text", 1, 1), get([1], 1, 1) and get(Dict(1=>2), 1, 1) are downright ambiguous.

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 31, 2018

get on Nullable was also a problem because it was slow, so we were adding crazy workarounds to deal with that. get with the Union approach does not suffer from that issue, and the workarounds are not needed

maleadt added a commit to JuliaGPU/CUDAnative.jl that referenced this pull request Jun 1, 2018
Keno pushed a commit that referenced this pull request Jun 5, 2024
* make `coalesce` handle only `missing`; add `something` to handle `nothing`/`Some`

fixes #26927

* Move something docs with those for nothing and Some
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:missing data Base.missing and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants