REGR: revert ExtensionBlock.set to be in-place

[jorisvandenbossche]

Alternative for #35266 and closes #35369

[jorisvandenbossche]

This is a more targeted revert (so a smaller change compared to #35266), but thus also not keeping the fix from #35266.

For 1.1, I however think it might be better to fix the regression in __setitem__ than to keep the fix in iloc. My reasoning: it seems worse to have a regression where an existing array is updated where it should not have been modiifed, as to have a bug where an existing array doesn't get updated where it should have been updated in place (in addition to that the bug already existed before).

[TomAugspurger]

cc @jreback and @jbrockmendel. Do you have a feeling for which of #35266 or this you want to proceed with for fixing #33457?

[jbrockmendel]

@TomAugspurger I'm planning to take a swing at this today.

[jreback]

this lgtm

[jbrockmendel]

@jreback pls put a hold on this pending resolution of #33457 (comment)

[simonjayhawkins]

This PR would also fix #35369

[jbrockmendel]

@simonjayhawkins would #35369 be addressed by removing the blk.should_store(value) case in BlockManager.iset? The discussion in #33457 is pointing towards that being the "correct" approach for this.

[simonjayhawkins]

@simonjayhawkins would #35369 be addressed by removing the blk.should_store(value) case in BlockManager.iset? The discussion in #33457 is pointing towards that being the "correct" approach for this.

yes, test added here passes with that patch.

[TomAugspurger]

@jbrockmendel I think this is the one to go with for 1.1.0. What are your thoughts?

[jbrockmendel]

Since this is un-fixing a tested bug in order to fix an un-tested bug, I'd prefer to keep this as-is until we get the longer-term solution in place (hopefully 1.1.1), but not a particularly strong preference.

[simonjayhawkins]

Since this is un-fixing a tested bug in order to fix an un-tested bug

FWIW, I think fixing regressions should take precedence. We don't know how many more users will be affected once 1.1 is out.

If we can do both and keep the bug fix great otherwise we should revert IMO.

[TomAugspurger]

Since this is un-fixing a tested bug in order to fix an un-tested bug, I'd prefer to keep this as-is

Which bugs are those? The "un-fixing" is the DataFrame.__setitem__ with Categorical dtype I think, but I'm not sure about the un-tested bug. And what does "as-is" mean here, 1.0.x or master?

@simonjayhawkins I'm interpreting your comment as a preference for this PR for 1.1.0, is that fair?

[simonjayhawkins]

@simonjayhawkins I'm interpreting your comment as a preference for this PR for 1.1.0, is that fair?

yes

[jbrockmendel]

Which bugs are those? The "un-fixing" is the DataFrame.setitem with Categorical dtype I think, but I'm not sure about the un-tested bug. And what does "as-is" mean here, 1.0.x or master?

The "un-fixing" is df.iloc[:, n] = foo, see the existing test that this PR xfails and the whatsnew note it removes.

The "un-tested bug" I'm referring to is the behavior that this PR fixes+tests, df[col] = foo.

ATM in master df[col] = foo consistently overwrites for all dtypes (conditional on should_store), while this PR (and 1.0.x) is inconsistent about it. I don't think that inconsistency is intentional, so it should be considered a bug.

By the same token, if we're not going to call df[col] = foo over-writing a bug for non-EA dtypes, then I don't think this should be considered a regression.

I think we're agreed that #35417 is the correct long-term fix. If, as @TomAugspurger suggested here, that needs to wait for 2.x, then I'd rather have internally consistent behavior in the interim (but I'd rather #35417 not wait for 2.x, in which case I'm OK with behavior being inconsistent for a shorter period)

[TomAugspurger]

meta-comment: I don't want to get too hung up on labeling these as "bug or not" or whether something is a regression; I'm more concerned with breaking people's workflows.

@jbrockmendel I'm still unsure what your preference is for specifically 1.1.0 (I longer-term is clearer). Is this OK to merge?

[jbrockmendel]

I'm still unsure what your preference is for specifically 1.1.0 (I longer-term is clearer). Is this OK to merge?

I'm OK with this conditional on #35417 not being held up until 2.0

[TomAugspurger]

OK, thanks.

I think I can be on board with "make DataFrame.__setitem__ consistently assigning a new array". I'll clarify / change my comment over in #33457 about it waiting for 2.x.