requested edit to comparison method

[jbrockmendel]

cc @jorisvandenbossche discussed in #27803

[jreback]

thanks, ideally add to this to a todo list

[jorisvandenbossche]

@jreback if it is a PR specifically for my comments, can you please give me a bit of time to review it?

There were actually more things we were discussing than this.

[jreback]

sure

[jorisvandenbossche]

@jbrockmendel what I wanted to ask in addition:

• can you add a test (for a non-frozenset case) for a case that would have broken by the previous PR?
• can update the wrong "# rename is needed in case res_name is None and result.name" comment?
• ideally, can you also do the rename only if needed? (or use a cheaper method that does not copy)

[jbrockmendel]

can you add a test (for a non-frozenset case) for a case that would have broken by the previous PR?

I'd rather not test not-necessarily-intentional behavior until we pin down the desired behavior in #27911

can update the wrong "# rename is needed in case res_name is None and result.name" comment?

Sure.

ideally, can you also do the rename only if needed? (or use a cheaper method that does not copy)

Doesn't it do a shallow copy? I guess creating a new Series object is more costly than the check in the case where it isnt needed? Wouldn't it be cheaper yet to unconditionally just set result.name = res_name?

[jorisvandenbossche]

I'd rather not test not-necessarily-intentional behavior

Well, it's working behaviour that people will rely upon, so we shouldn't just break it IMO. So in that case a test would be useful. It would eg have catched the changes you were doing in the previous PR, and will do if you or someone further work on that code in the future (we are now well aware, but if we only pick it up again in some months, we might well have forgotten again)

Doesn't it do a shallow copy?

Nope, it returns a new object, which currently means copying the data (we don't yet have a copy-on-write)

[jorisvandenbossche]

Well, it's working behaviour that people will rely upon, so we shouldn't just break it IMO.

But will take a look at the issue you linked

[jorisvandenbossche]

I'd rather not test not-necessarily-intentional behavior

And it would also have helped to catch https://github.com/pandas-dev/pandas/pull/27769/files#r314060256 :-)