MAINT: Fix for pandas pre #12347
MAINT: Fix for pandas pre #12347
Conversation
larsoner
requested review from
agramfort,
dengemann,
drammock and
mscheltienne
as code owners
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
Self-merging to keep things clean but @drammock @hoechenberger if you see a cleaner fix feel free to comment!
| ] | ||
|
|
||
| data = np.empty((len(events_df), len(columns))) | ||
| data = np.empty((len(events_df), len(columns)), float) |
There was a problem hiding this comment.
Just to be explicit that we initialize with float dtype for our dataframe
|
|
||
| # Event names | ||
| metadata.iloc[:, 0] = "" | ||
| metadata["event_name"] = "" |
There was a problem hiding this comment.
... and Pandas does not seem to complain when you set an entire col (or list of cols) rather than using .iloc, so I go that route here
|
This would be an important backport candidate, because the issue affects pandas >= 2.1.0 (so not just pre-releases). But I guess the 1.7 release is around the corner and it's not worth it? If it's more than three weeks out, a bugfix release would probably be a good idea. WDYT? |
| # keep_first and keep_last names | ||
| start_idx = stop_idx | ||
| metadata.iloc[:, start_idx:] = None | ||
| metadata[columns[start_idx:]] = "" |
There was a problem hiding this comment.
I'm not sure if this is guaranteed to work in-place (now and in the future). The cleaner approach for this specific case here is definitely through using .loc or iloc
There was a problem hiding this comment.
I don't think that this is a problem here; usually, the infamous SettingWithCopyWarning appears only after chaining two or more indexing operations (https://pandas.pydata.org/docs/user_guide/indexing.html#indexing-view-versus-copy).
Also, the previous line triggered the FutureWarning – do you have an alternative suggestion how to typecast the column (to object) and then set its entries to None?
There was a problem hiding this comment.
In case I'm wrong, do you have a link for a list of channel labels not being safe?
There was a problem hiding this comment.
Ok let's keep it, then :)
When you say it affects those versions of Pandas, do you mean it emits a FutureWarning (which wouldn't be worth a backport release) or because it actually breaks / emits an error / does something incorrect (which would be)? I'm guessing it's the former unless pandas deprecation cycle is even shorter than ours |
|
Yes, it just emits a |
3 participants
Closes #12346
Not a pandas expert but
mne/tests/test_epochs.pypasses on my machine now andmetadata.dtypesand contents look reasonable at first look