-
Notifications
You must be signed in to change notification settings - Fork 15
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
Avoid Union{Missing,T}
in columns after DropMissing
#35
Conversation
I think this current implementation has the issue that it forces a Union{Missing,T} in the revert even when the original table doesn't have missing values. As we discussed over Zulip, we just need to save the original column eltype during the apply and reuse it in the revert. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix the revert so that it restores the original eltype, and also add tests.
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
==========================================
+ Coverage 92.51% 92.57% +0.06%
==========================================
Files 16 16
Lines 414 431 +17
==========================================
+ Hits 383 399 +16
- Misses 31 32 +1
Continue to review full report at Codecov.
|
Done. I made the requested changes and added tests @juliohm. |
@juliohm, I moved post and pre processing to apply and revert. This avoids creating the helpers functions. _nonmissing(::Type{T}, c) where {T} = c
_nonmissing(::Type{Union{Missing,T}}, c) where {T} =
collect(T, c)
function _nonmissing(columns, col)
c = Tables.getcolumn(columns, col)
_nonmissing(eltype(c), c)
end But I can simplify this function, but the calls to the collect function will happen in all columns of the table: function _nonmissing(columns, col)
c = Tables.getcolumn(columns, col)
collect(nonmissingtype(eltype(c)), c)
end Which of the two options do you prefer? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor changes now and I think we are ready
This PR is ready for a final review @juliohm! |
This PR adds a post-processing in the apply function of the DropMissing transformation and a pre-processing in the revert function.
This is necessary so that the type of columns where the DropMissing transformation will be applied is
T
instead ofUnion{Missing,T}
.