[ArrayManager] BUG: fix setitem with non-aligned boolean dataframe
State: closed

xref indexing work item of https://github.com/pandas-dev/pandas/issues/39146

This covers setitem with a boolean dataframe (like ``df[df < 0] = 2``), but specifically fixes the case where the dataframe is not aligned (eg `df[df[:-1] < 0] = 2` in the tests). 

`DataFrame.fillna` infers object dtype columns getting filled for a proper dtype, but with ArrayManager internals I propose that `fillna` always preserves the dtype.


Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
Might be a coincidence, but in the ArrayManager code `def setitem` is commented out with a `# TODO what is this used for?`.  The answer is it is used for setting via iloc
> Might be a coincidence, but in the ArrayManager code `def setitem` is commented out with a `# TODO what is this used for?`. The answer is it is used for setting via iloc

I was actually also implementing that while trying to get tests in `frame/indexing/test_indexing.py` passing, but since this `where` related fix and the `setitem` implementation are not actually related, split it off in a separate branch (but will be opening a PR for it)
@jbrockmendel more comments here? (suggestions for a cleaner solution / where to put this are certainly welcome)
will take another look this afternoon
Big picture: what was your takeaway from last week's call re behavior differences between ArrayManager vs BlockManager.  My takeaway was that wherever feasible we should keep them in sync and do a deprecation on the existing method.

Is there a technical reason why keeping the behaviors in sync is not feasible?
>  My takeaway was that wherever feasible we should keep them in sync and do a deprecation on the existing method.

My interpretation was different (but it was not fully clear to me what the general sense was about this / what the conclusion was, so maybe this is rather stating what I would argue for myself): have new behaviour in ArrayManager, deprecate in BlockManager.  

If we agree we want to deprecate a certain behaviour (and thus in practice deprecate it for the current default internals, i.e. using BlockManager), IMO it doesn't make much sense to implement the old deprecated behaviour for ArrayManager: 1) this will not always be easy (for this case of fillna downcasting that's not a problem, but eg I don't want to try to replicate the copy/view behaviours of the BlockManager in ArrayManager), 2) having the new behaviour in ArrayManager actually makes it possible to experiment with this (which I think will be useful). 
Implementing the future behaviour now also ensures we can have a good overview and implementation of changing behaviour for a potential 2.0 (or 3.0) release, instead of having to do that just before that release.

Of course, specifically for the `fillna` downcasting behaviour, that is quite self contained and thus rather easy to add in ArrayManager as well (there is certainly no technical reason that it's not possible here). https://github.com/pandas-dev/pandas/pull/39578 is another example of changing behaviour, and one where I think it is much less straightforward (or desired) to mimic behaviour of BlockManager. 

It also might not need to be all one way or the other. We could also decide on a case by case, and eg prefer preserving behaviour for fillna while implementing new behaviour for eg indexing operations.
> My interpretation was different (but it was not fully clear to me what the general sense was about this / what the conclusion was, so maybe this is rather stating what I would argue for myself): 

yah i checked the notes and it doesnt look like we wrote anything down.  easy for it to become a rorscach test

> If we agree we want to deprecate a certain behaviour (and thus in practice deprecate it for the current default internals, i.e. using BlockManager)

The "if" part is important.  That discussion should be orthogonal to the "getting ArrayManager fully functional" task.

> this will not always be easy (for this case of fillna downcasting that's not a problem, but eg I don't want to try to replicate the copy/view behaviours of the BlockManager in ArrayManager)

No objection on the copy/view-indexing bit.  But for other methods where it _is_ easy i think the default should be to keep behavior in sync.

This will certainly reduce complexity in the test suite, and I'm guessing in the code too.  e.g. am I right in thinking that the extra code in `NDFrame._where` (that both Jeff and I have independently commented on asking if it can go in ArrayManager) would be easier to keep in ArrayManager if it weren't for the changed behavior?

> having the new behaviour in ArrayManager actually makes it possible to experiment with this (which I think will be useful).

When evaluating ArrayManager, it is important that we evaluate it in isolation.  e.g. if I see a perf change I don't want to have to wonder/check if it is due to a type inference being skipped or something.
> yah i checked the notes and it doesnt look like we wrote anything down. easy for it to become a rorscach test

Yeah, it was the first thing I thought after our meeting "we should have forced ourselves to write down some conclusions, because I already don't know anymore what those were" ..

> The "if" part is important. That discussion should be orthogonal to the "getting ArrayManager fully functional" task.

Yes, I know. That's the reason I opened https://github.com/pandas-dev/pandas/issues/39584, and I know I need to open one for the downcasting behaviour as well.

> e.g. am I right in thinking that the extra code in NDFrame._where (that both Jeff and I have independently commented on asking if it can go in ArrayManager) would be easier to keep in ArrayManager if it weren't for the changed behavior?

If we don't change behaviour, there is nothing to change at all here. It's only *if* we change behaviour, that *some* part of the changes need to be handled in `DataFrame._where`. If `fillna` doesn't change behaviour, no change is needed here. 
When I am saying "I don't directly see an easy way to handle this inside ArrayManager", that is only for the case of a changed `fillna` behaviour (which was initially my starting point for this PR).


> When evaluating ArrayManager, it is important that we evaluate it in isolation. e.g. if I see a perf change I don't want to have to wonder/check if it is due to a type inference being skipped or something.

I am OK with keeping the fillna behaviour as is for now. So we can keep the discussion about keeping ArrayManager consistent or not for the harder changes such as indexing behaviour.
(but note that the above won't be possible in general, because also eg copy/view differences between both will have some impact)

So closing this PR, will open a new one making `fillna` consistent for ArrayManager.
-> new PR at https://github.com/pandas-dev/pandas/pull/39697