Add generic isnull and unsafe_get methods by davidagold · Pull Request #18484 · JuliaLang/julia

davidagold · 2016-09-13T18:04:45Z

It would be useful to have a generic fallback for isnull when operating on iterators (usually tuples) of both Nullable and non-Nullable objects. Is there anything else we need to do here to make sure this is optimized away in the case of non-Nullable arguments? What should tests look like?

cc: @johnmyleswhite @nalimilan @quinnj

johnmyleswhite · 2016-09-13T18:08:20Z

I'm not sure how Base wants to handle things, but I would say that the removal of a dead branch in code like the following is what we require:

function loop(x::Vector)
    s = 0.0
    for x_i in x
        if !isnull(x_i)
            s += x_i
        end
    end
    return s
end

code_llvm(loop, (Vector{Float64}, ))

davidagold · 2016-09-13T18:22:12Z

That makes sense. I will leave that assessment to somebody who can read LLVM code.

This PR also seems an appropriate place to introduce an analogous method for the value field of a Nullable:

unwrap(x::Nullable) = x.value
unwrap(x) = x

as in John's jplyr notes.

johnmyleswhite · 2016-09-13T18:24:43Z

I think we definitely want something like unwrap, although we might want to call it something like unsafe_get since it is essentially a version of get(x::Nullable) that is unsafe, but extended to work on non-nullables (in a way that ironically ends up being safe).

nalimilan · 2016-09-13T19:53:11Z

Regarding unsafe_unwrap, maybe rename get to unwrap by the way?

eschnett · 2016-09-13T20:22:43Z

In other contexts (e.g. C++ futures), unwrap is a slightly different operation: It converts Nullable{Nullable{T}} to Nullable{T}, which is always safe. That is, it peels off one layer if there are multiple layers.

TotalVerb · 2016-09-13T20:29:07Z

LLVM can already eliminate the branch, but inference cannot. See Dead branch elimination after inlining #17880

davidagold · 2016-09-18T01:26:03Z

Should this wait for or become part of #18510 ?

nalimilan · 2016-09-18T13:01:13Z

Should this wait for or become part of #18510 ?

The two changes can be made independently, so we can merge any of them we want.

nalimilan · 2016-09-27T08:02:48Z

@davidagold Should probably add a test for the new method? Then I vote for merging.

davidagold · 2016-09-28T19:30:26Z

Ready for review.

nalimilan · 2016-09-29T08:09:05Z

test/nullable.jl

+
+@test_throws UndefRefError unsafe_get(Nullable())
+
+# unsafe_get(x)


Could as well merge this loop with the one above.

I suppose? But it doesn't need to be executed multiple times.

EDIT: Oh wait, misread this. Yes, I agree.

nalimilan · 2016-09-29T08:09:42Z

test/nullable.jl

+    @test unsafe_get(x4) === a
+end
+
+@test_throws UndefRefError unsafe_get(Nullable())


Maybe also test with another type, e.g. Nullable{String}()?

nalimilan · 2016-09-29T08:09:53Z

base/nullable.jl


 get(x::Nullable) = x.isnull ? throw(NullException()) : x.value

+unsafe_get(x::Nullable) = x.value


Needs a docstring.

nalimilan · 2016-09-29T08:11:43Z

test/nullable.jl

 @test isnull(Nullable())

+# isnull(x)
+for T in types


davidagold · 2016-09-29T16:01:22Z

I've added a docstring for unsafe_get added, modified the isnull docstring, and moved the latter from docs/helpdb/Base.jl to base/nullable.jl. I've also incorporated Milan's suggestions.

nalimilan · 2016-09-29T16:11:06Z

base/nullable.jl

+
+Return the value of `x` for `x::Nullable`; return `x` for all other `x`.
+
+Unsafe because does not check whether or not `x` is null before attempting to


Maybe "This function is unsafe because it does not..."?

davidagold · 2016-09-29T18:41:20Z

@nalimilan How's this?

nalimilan · 2016-09-29T18:47:14Z

base/nullable.jl

+"""
+    unsafe_get(x)
+
+Return the value of `x` for [`Nullable`](:obj:`Nullable`) `x`; return `x` for


Just being curious, do we need this to get links?

That's my understanding. @kshyatt included these in https://github.com/JuliaLang/julia/pull/18511/files#diff-1955b2684af6e5a407aa36281f8bafdeR94, so I figured I'd include them here, too.

Is this documented somewhere? Maybe somebody could add a word about this at http://docs.julialang.org/en/latest/manual/documentation/? @MichaelHatherly

(FWIW, the repetition of the object name sounds a bit annoying to me...)

That syntax is being removed in #18588 in favour of [Nullable](@ref), so probably no need to add it to the docs at this point.

so I figured I'd include them here, too.

We can never have too many cross references 👍

Okay, I'll remove the references for now. I think we will all live with the repetition in the docstring.

does the auto conversion script translate them from the current rst-in-markdown hybrid to the new style? if so it won't hurt to add if this gets merged before the conversion

Yes, handled automatically. It's fine to keep adding xrefs.

Okay, I will add it back in.

Also move isnull docstring out of helpdb and into base/nullable.jl.

davidagold · 2016-10-01T21:40:19Z

I think everything is good here. @nalimilan ?

tkelman · 2016-10-02T00:09:00Z

These docstrings need to be added to the manual if this is exported

tkelman · 2016-10-02T00:33:41Z

This was also missing a make docs update to the modified docstring. That should be done at the same time, or it will end up in someone else's unrelated PR.

tkelman · 2016-10-03T07:35:09Z

base/nullable.jl

+isnull(x::Nullable) = x.isnull
+isnull(x) = false
+
 isnull(x::Nullable) = !x.hasvalue


davidagold · 2016-10-03T14:12:51Z

Yeesh, who knew I was such a scrub. I'll fix this and the documentation bit
today.

On Monday, October 3, 2016, Tony Kelman notifications@github.com wrote:

@tkelman commented on this pull request.

In base/nullable.jl
#18484 (review):

+julia> x = Nullable(1, true)
+Nullable{Int64}()
+
+julia> isnull(x)
+true
+
+julia> x = 1
+1
+
+julia> isnull(x)
+false
+```
+"""
+isnull(x::Nullable) = x.isnull
+isnull(x) = false
+
isnull(x::Nullable) = !x.hasvalue

bad rebase

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#18484 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALUCm5Bkx2EbY7dqHH6AKbyzmRrXpEGsks5qwLA_gaJpZM4J7-CG
.

davidagold · 2016-10-04T00:30:27Z

@tkelman should I revert this or put those fixes in a new PR?

EDIT: Also, what precisely do you mean by "add docstrings to the manual" ?

tkelman · 2016-10-04T00:36:53Z

little cleanup in a new pr is all that's needed, it's not that bad

Compat for Julia #18510, #18484

kshyatt added the missing data Base.missing and related functionality label Sep 13, 2016

tkelman added the needs tests Unit tests are required for this change label Sep 18, 2016

davidagold force-pushed the patch-1 branch from feddd37 to bf60636 Compare September 28, 2016 19:29

tkelman removed the needs tests Unit tests are required for this change label Sep 28, 2016

davidagold force-pushed the patch-1 branch 2 times, most recently from 9356b27 to fb205d2 Compare September 29, 2016 00:46

nalimilan reviewed Sep 29, 2016

View reviewed changes

test/nullable.jl Outdated

@test isnull(Nullable())

# isnull(x)

for T in types

Copy link
Copy Markdown

Member

nalimilan Sep 29, 2016

Choose a reason for hiding this comment

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

Same here.

davidagold force-pushed the patch-1 branch from fb205d2 to 7a3d9bc Compare September 29, 2016 15:57

nalimilan reviewed Sep 29, 2016

View reviewed changes

davidagold force-pushed the patch-1 branch 2 times, most recently from e64b787 to ef546f5 Compare September 29, 2016 18:40

nalimilan reviewed Sep 29, 2016

View reviewed changes

davidagold changed the title ~~Add generic isnull method~~ Add generic isnull and unsafe_get methods Sep 29, 2016

nalimilan approved these changes Sep 29, 2016

View reviewed changes

davidagold force-pushed the patch-1 branch from ef546f5 to 33da470 Compare September 29, 2016 20:06

davidagold force-pushed the patch-1 branch from 33da470 to 8380cca Compare September 29, 2016 20:59

Add generic isnull, unsafe_get methods

8380cca

Also move isnull docstring out of helpdb and into base/nullable.jl.

nalimilan merged commit 35b2583 into JuliaLang:master Oct 1, 2016

davidagold mentioned this pull request Oct 1, 2016

WIP: Nullable lifting infrastructure #18758

Closed

davidagold deleted the patch-1 branch October 1, 2016 22:22

tkelman added the needs docs Documentation for this change is required label Oct 2, 2016

tkelman reviewed Oct 3, 2016

View reviewed changes

base/nullable.jl

isnull(x::Nullable) = x.isnull

isnull(x) = false

isnull(x::Nullable) = !x.hasvalue

Copy link
Copy Markdown

Contributor

tkelman Oct 3, 2016

Choose a reason for hiding this comment

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

bad rebase

This was referenced Oct 5, 2016

Couple of small docs fixes #18784

Merged

Fix issues noted in 18484 #18824

Merged

tkelman removed the needs docs Documentation for this change is required label Oct 7, 2016

davidagold mentioned this pull request Oct 17, 2016

Compat for Julia #18510, #18484 JuliaLang/Compat.jl#287

Merged

StefanKarpinski pushed a commit that referenced this pull request Feb 8, 2018

Compat for Julia #18510, #18484

11b9210

StefanKarpinski pushed a commit that referenced this pull request Feb 8, 2018

Merge pull request #287 from davidagold/dag/nullable

2534435

Compat for Julia #18510, #18484


		@test_throws UndefRefError unsafe_get(Nullable())

		# unsafe_get(x)


		get(x::Nullable) = x.isnull ? throw(NullException()) : x.value

		unsafe_get(x::Nullable) = x.value


		Return the value of `x` for `x::Nullable`; return `x` for all other `x`.

		Unsafe because does not check whether or not `x` is null before attempting to

Uh oh!

Conversation

davidagold commented Sep 13, 2016

Uh oh!

johnmyleswhite commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidagold commented Sep 13, 2016

Uh oh!

johnmyleswhite commented Sep 13, 2016

Uh oh!

nalimilan commented Sep 13, 2016

Uh oh!

eschnett commented Sep 13, 2016

Uh oh!

TotalVerb commented Sep 13, 2016

Uh oh!

davidagold commented Sep 18, 2016

Uh oh!

nalimilan commented Sep 18, 2016

Uh oh!

nalimilan commented Sep 27, 2016

Uh oh!

davidagold commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidagold Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidagold commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidagold commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidagold commented Oct 1, 2016

Uh oh!

tkelman commented Oct 2, 2016

Uh oh!

tkelman commented Oct 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidagold commented Oct 3, 2016

@tkelman commented on this pull request.

Uh oh!

davidagold commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tkelman commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

johnmyleswhite commented Sep 13, 2016 •

edited

Loading

davidagold Sep 29, 2016 •

edited

Loading

davidagold commented Oct 4, 2016 •

edited

Loading