Skip to content

Conversation

@jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel commented Mar 3, 2020

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

xref #32414.

cc @TomAugspurger ATM the new test implemented in tests.arrays.categorical.test_missing is failing on the ndarray case for reasons that I dont fully grok. Are you familiar with this?

@TomAugspurger
Copy link
Contributor

I think the failing test case is consistent with accidentally using a code for a value? Perhaps test with a Categorical and ndarray containing something other than integers to see if that's the case.

@jbrockmendel
Copy link
Member Author

Looks like the problem is that ATM Categorical.fillna is altering in-place for Series other

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm, but a question

return comps._values.isin(values)

comps = com.values_from_object(comps)
return comps.isin(values) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this type ignore?

Copy link
Member Author

Choose a reason for hiding this comment

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

bc mypy doesnt know enough to rule out ndarray (or even non-Categorical EA) here

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure i understand. shouldn't you just add

result = comps.isin(values)
assert isinstance(result, np.ndarray)
return result

prefer this to a type ignore

Copy link
Member Author

Choose a reason for hiding this comment

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

the issue isnt type(result), its type(comps). Categorical has a isin method, but general ExtensionArray does not

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, is there a way to avoid the type ignore, i hate these things because we never remove them

Copy link
Contributor

Choose a reason for hiding this comment

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

what about an isinstance(ABCCategorical) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

or ok with a run-time import of Categorical if the above doesn't work;

Copy link
Member Author

Choose a reason for hiding this comment

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

mypy doesnt recognize any of our ABCFoos

Copy link
Member Author

Choose a reason for hiding this comment

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

so you have an aversion to "# type: ignore" and i have an aversion to a runtime import just for mypy. is there a middle ground somewhere? maybe revisit in a few months to see if the type ignore can be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's do that, also let's create an issue to see if we can fix the ABC's and mypy

@jreback jreback added Categorical Categorical Data Type Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Mar 8, 2020
@jreback jreback added this to the 1.1 milestone Mar 11, 2020
@jbrockmendel
Copy link
Member Author

gentle ping; this is a blocker for tightening types (and avoiding unwanted code patterns) in Block.putmask and Block.where

@jreback jreback merged commit fd2e002 into pandas-dev:master Mar 14, 2020
@jreback
Copy link
Contributor

jreback commented Mar 14, 2020

thanks, i think we need a whatsnew note as well, pls do in a followon

@jbrockmendel jbrockmendel deleted the fillna branch March 14, 2020 16:52
SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Categorical Categorical Data Type Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants