JuliaArrays / PaddedViews.jl

Add virtual padding to the edges of an array
Other
49 stars 9 forks source link

loose eltype when fillvalue is nothing or missing #24

Closed johnnychen94 closed 4 years ago

johnnychen94 commented 4 years ago

Closes #19

Because for Nothing and Missing, T isn't the eltype anymore, we have to add several methods(eltype, similar) to support it.

This might be good to go, but I'll need to check it more carefully. I was hoping to provide a patch before anyone sees the updates in #19.

codecov-io commented 4 years ago

Codecov Report

Merging #24 into master will increase coverage by 0.66%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   71.42%   72.09%   +0.66%     
==========================================
  Files           1        1              
  Lines          42       43       +1     
==========================================
+ Hits           30       31       +1     
  Misses         12       12
Impacted Files Coverage Δ
src/PaddedViews.jl 72.09% <100%> (+0.66%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1762776...7bb1d70. Read the comment docs.

timholy commented 4 years ago

I don't think you have to convert anything; you can just use

T = eltype(data)
T = fillvalue isa Union{Missing,Nothing} ? Union{typeof(fillvalue),T} : T

in the constructor and use that for the first type parameter of PaddedView.

johnnychen94 commented 4 years ago

🤔 looks like you're right; the padded value doesn't come from data so we don't need to modify data.

Will change accordingly when I find some time.

haberdashPI commented 4 years ago

That was fast, thanks so much for working on this! Much appreciated.

johnnychen94 commented 4 years ago

I think the revision commit does exactly what @timholy indicated, so I'm merging it now

timholy commented 4 years ago

Yep, perfect to the letter. Really nice work! And thanks for doing it. :heart: