Skip to content
This repository has been archived by the owner on May 4, 2019. It is now read-only.

Fix broadcast for 0.5 #123

Merged
merged 1 commit into from
Jun 23, 2016
Merged

Fix broadcast for 0.5 #123

merged 1 commit into from
Jun 23, 2016

Conversation

davidagold
Copy link
Contributor

This PR transitions the NullableArrays broadcast interface to incorporate that present in base: https://github.com/JuliaLang/julia/blob/913b24de9c68b6c79a767bc2f6caacea249b3198/base/broadcast.jl. Functionality remains the same. I made one minor revision to the tests, which originally checked (for some reason) that broadcast! and map! give identical behavior when fed only a single array argument. They don't -- see JuliaLang/julia#12277. The fact that those tests were ever not problematic means that the semantics of broadcast specialized for NullableArray arguments were not consistent with those in base. Now they are.

@quinnj
Copy link
Member

quinnj commented Jun 21, 2016

Hmmm, looks like this breaks 0.4. We also still have a few other fixes needed for 0.5 (which is why that check failed).

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.07%) to 66.22% when pulling 05e1bf0 on broadcast into 1616e96 on master.

@davidagold
Copy link
Contributor Author

The push failed because I hadn't pulled Milan's fixes for the other stuff. The PR failed because I forgot I couldn't just nix all the old definitions >_> Fixing that now.

@codecov-io
Copy link

codecov-io commented Jun 21, 2016

Current coverage is 82.96%

Merging #123 into master will increase coverage by 4.67%

@@             master       #123   diff @@
==========================================
  Files            13         13          
  Lines           677        716    +39   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            530        594    +64   
+ Misses          147        122    -25   
  Partials          0          0          

Powered by Codecov. Last updated by 1616e96...77de5a9

@coveralls
Copy link

coveralls commented Jun 21, 2016

Coverage Status

Coverage increased (+4.7%) to 82.961% when pulling 87af2ae on broadcast into 1616e96 on master.

@davidagold
Copy link
Contributor Author

"Rebasing" this in a crude yet (hopefully effective) manner.

@coveralls
Copy link

coveralls commented Jun 21, 2016

Coverage Status

Coverage increased (+4.7%) to 82.961% when pulling 77de5a9 on broadcast into 1616e96 on master.

@davidagold
Copy link
Contributor Author

Comments/suggestions @quinnj @nalimilan ?

nullcheck
end

@generated function Base.Broadcast._broadcast!{M,XT,nargs}(f, Z::NullableArray, indexmaps::M, Xs::XT, ::Type{Val{nargs}}; lift=false)
Copy link
Member

Choose a reason for hiding this comment

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

Cut line at 92 chars.

@nalimilan
Copy link
Member

Cool. I could only make superficial comments. Please merge and tag a new release as soon as you can!

@quinnj
Copy link
Member

quinnj commented Jun 22, 2016

Travis is happy :)

Transition to broadcast interface in
JuliaLang/julia@522547b
@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage increased (+4.9%) to 83.148% when pulling 4dec5b9 on broadcast into 1616e96 on master.

@davidagold
Copy link
Contributor Author

davidagold commented Jun 23, 2016

@quinnj @nalimilan I don't know what to make of the push failure. I can't recreate it locally. Possibly just some floating point weirdness? Though I don't see how that would be possible...

EDIT: It's especially weird, since the push and pr versions should be identical.

@quinnj
Copy link
Member

quinnj commented Jun 23, 2016

I restarted the build. I'll also pull locally to see if I can reproduce.

@quinnj
Copy link
Member

quinnj commented Jun 23, 2016

Must have been intermittent; it all looks good now.

@quinnj quinnj merged commit 61398f3 into master Jun 23, 2016
@quinnj quinnj deleted the broadcast branch June 23, 2016 17:36
@quinnj
Copy link
Member

quinnj commented Jun 23, 2016

Hmmm.....any idea what the map failure is, @davidagold? https://travis-ci.org/JuliaStats/NullableArrays.jl/jobs/139840111

@davidagold
Copy link
Contributor Author

davidagold commented Jun 23, 2016

... No? When did that happen?

Edit: master seems fine locally...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants