Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can we change the expected value instead of this part? I think it's good to know how this function handles
None
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.
We can't really do it, since there are two distinct behaviors for this input, depending on what version of pandas we are running. For
pandas 1.4.0
, the existing code doesn't even work on this input. Notice that the categorical transformer does the following:RDT/rdt/transformers/categorical.py
Lines 106 to 109 in 40d2667
So we actually just overwrite the first
np.nan
with theNone
, which is not the intended behavior. If we do want to keep support for multiple types of nan's, we could cast all null objects nonp.nan
right at the start of the function.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.
It's weird that
value__counts
doesn't treatNone
asnp.nan
, butpd.isna
will still returnTrue
for it. My question is, is it possible for_get_intervals
to receive data with different null types. If so, we have to handle that which means I think we should convert them all before doing thevalue_counts