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

loose eltype when fillvalue is nothing or missing #24

Merged
merged 2 commits into from
Mar 9, 2020
Merged

loose eltype when fillvalue is nothing or missing #24

merged 2 commits into from
Mar 9, 2020

Conversation

johnnychen94
Copy link
Member

@johnnychen94 johnnychen94 commented Mar 7, 2020

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
Copy link

codecov-io commented Mar 7, 2020

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%) ⬆️

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.

src/PaddedViews.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Member

timholy commented Mar 7, 2020

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
Copy link
Member Author

🤔 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.

@johnnychen94 johnnychen94 changed the title smartly loose eltype for nothing and missing loose eltype when fillvalue is nothing or missing Mar 8, 2020
indices::NTuple{N,AbstractUnitRange}) where {T,N,I,A}
ndims(data) == N || throw(DimensionMismatch("data and indices should have the same dimension, instead they're $(ndims(data)) and $N."))
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we can't ensure that eltype(data) == T

@haberdashPI
Copy link

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

@johnnychen94
Copy link
Member Author

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

@johnnychen94 johnnychen94 merged commit 244f397 into JuliaArrays:master Mar 9, 2020
@johnnychen94 johnnychen94 deleted the jc/nothing branch March 9, 2020 15:39
@timholy
Copy link
Member

timholy commented Mar 9, 2020

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

johnnychen94 added a commit that referenced this pull request Mar 20, 2020
This continues #24 by make filltype conversion extensible to other
packages, e.g., it makes lifting filltype for `Colorants` possible
in other packages.

This is a rework/breaking PR of #25
johnnychen94 added a commit that referenced this pull request Mar 20, 2020
This continues #24 by make filltype conversion extensible to other
packages, e.g., it makes lifting filltype for `Colorants` possible
in other packages.

This is a rework/breaking PR of #25
johnnychen94 added a commit that referenced this pull request Apr 2, 2020
This continues #24 by make filltype conversion extensible to other
packages, e.g., it makes lifting filltype for `Colorants` possible
in other packages.

This is a rework/breaking PR of #25
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.

Support missing values
4 participants