API: harmonize Index.to_flat_index to MultiIndex.to_flat_index #25094
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25094 +/- ##
==========================================
+ Coverage 92.37% 92.37% +<.01%
==========================================
Files 166 166
Lines 52420 52421 +1
==========================================
+ Hits 48423 48424 +1
Misses 3997 3997
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25094 +/- ##
==========================================
+ Coverage 92.34% 92.35% +<.01%
==========================================
Files 166 166
Lines 52314 52315 +1
==========================================
+ Hits 48311 48313 +2
+ Misses 4003 4002 -1
Continue to review full report at Codecov.
|
doc/source/whatsnew/v0.24.1.rst
Outdated
| @@ -69,6 +69,9 @@ Fixed Regressions | |||
|
|
|||
| - Fixed the warning for implicitly registered matplotlib converters not showing. See :ref:`whatsnew_0211.converters` for more (:issue:`24963`). | |||
|
|
|||
| **MultiIndex** | |||
|
|
|||
| - :meth:`Index.to_flat_index` now always returns an :class:`Index` containing tuples, even in the case the original object was already a flat :class:`Index` (:issue:`23670`). | |||
There was a problem hiding this comment.
this is a significant api change, so would go in 0.25.0. IF we do this (need discussions).
There was a problem hiding this comment.
This should have been part of #22866, or at least of 0.24.0, when the method was introduced from scratch. Too late for that, but let's at least not expose for long a behavior which is inconsistent. Sure, this needs discussion ;-)
There was a problem hiding this comment.
So you mean to say Index.to_flat_index is not idempotent?
There was a problem hiding this comment.
why is this a good thing though?
There was a problem hiding this comment.
For all the reasons in the discussion, plus one: one_level_multi_index.to_flat_index() is not idempotent, either.
A bit late to the party here, but I still don't understand the need for flattening MIs. I looked at the original issue and the usecase there (squashing MI-columns for csv output) could IMO more idiomatically be solved by something like Of course, the latter current does not work yet (see also #23679), but I continue to think that tuples should not be usable in indexes. They are already necessary to "talk" to MIs, which makes their use ambiguous, plus it's incredibly hard to get right (and we don't, for large parts of the code base, including basic indexing), not least because it needs so much special casing for interacting with numpy (which treats tuples as arrays by default). See #24688 |
Will comment on #24688 in few seconds. But this PR assumes that |
No, this PR doesn't change 1 level
This is quite clear in the issue: #23670 Anyway, in short, it's really simple:
This is really a simple thing, even @WillAyd is now neutral. Please provide any reason not to merge this, or let's proceed. |
toobaz
force-pushed
the
flat_index_to_flat_index_23670
branch
from
9070163
to
f7935c2
Compare
|
Closing for now - I think it's worth continuing conversation in issue before updating any PRs on this |
to_flat_indexon flatIndex#23670git diff upstream/master -u -- "*.py" | flake8 --diffAfter "implementing" it (that is, just unifying the method), I'm even more convinced about my idea... but @WillAyd and @h-vetinari feel free to consider this just a stimulus to resurrect (and conclude) the discussion ;-)