Repository navigation
Fix pandas-3 crash in get_contained_labels: set_index on string .values - #575
Conversation
|
Just two quick notes after testing on pandas 3.1:
label_map = label_map.set_index("symbol", drop=False)
label_map = label_map.set_index("description", drop=False) |
|
Follow-up: while validating this fix downstream (pandas 3.1.0, wfdb 4.3.1), I noticed that |
|
Thanks for the review! Both points are now addressed on this branch:
The remaining |
…db shim Second review note on MIT-LCP/wfdb-python#575 (cbrnr): pandas 3.1 deprecates the inplace keyword of DataFrame.set_index (Pandas4Warning, removed in pandas 4) -- the shim would have broken on pandas 4. Convert all four set_index calls in the patched method to the reassignment form; the shim now emits no deprecation warnings of its own (the one remaining Pandas4Warning in a get_contained_labels call chain comes from wfdb's own set_label_elements, outside this shim's method). Output schema and values re-verified identical to the pandas 2.3 reference.
|
Yes, please remove all |
|
Done in ccdf1fc -- all six Verified on pandas 3.1.0: |
|
There's one more Also, can you rebase please? |
…ues)
Under pandas >= 3 (Arrow-backed strings), DataFrame.set_index rejects
the ArrowStringArray produced by <string column>.values, so
get_contained_labels raises TypeError whenever the annotation carries
symbol or description. Pass the column names instead, with
drop=False so the symbol/description column survives in the returned
contained_labels (the old array-index path kept every column; plain
set_index("symbol") would consume it).
This is MIT-LCP#575 rebased onto the restructured main: main has since moved
to the src layout and independently converted all
set_index(inplace=True) call sites to reassignments, so the remaining
diff is exactly this two-line fix. Verified on pandas 3.1.0:
rdann -> get_contained_labels -> set_label_elements emits zero
Pandas4Warning and the contained_labels schema is unchanged.
ccdf1fc to
df9ccd7
Compare
|
Rebased onto the restructured main (df9ccd7). Since the branch was last updated, main moved to the src layout and independently converted all label_map = label_map.set_index("symbol", drop=False) # was label_map["symbol"].values
label_map = label_map.set_index("description", drop=False) # was label_map["description"].values( The crash itself is still reproducible on current main under pandas 3.1.0 ( |
|
@tompollard please merge if you're happy - I'm not sure about a changelog entry? |
No test called get_contained_labels, so the crash with pandas 3 was not caught. The test reads an annotation with symbols only, as rdann returns by default, and fails on pandas 3 without the fix.
|
Thanks, looks good. I checked it with pandas 3.1 and 2.2.3: the crash is fixed, and the output is identical to before on pandas 2. Yes, I think it's worth a changelog entry, since @cbrnr thanks for your reviews on this! |
|
Very nice! Thanks @wenh06 and @tompollard! |
Follow-up to #561 (v4.3.1). As analyzed in #574 (with a correction below), the module-level import crash is indeed fixed, but
get_contained_labelsstill passes string columns'.valuesdirectly toset_index:label_map.set_index(label_map["symbol"].values, inplace=True)label_map.set_index(label_map["description"].values, inplace=True)Under pandas' new string dtype (pandas 3.0, or 2.x with
future.infer_string),Series.valuesreturns anArrowStringArrayNumpySemantics, whichset_indexrejects withTypeError. Passing the column label is equivalent on every pandas version and immune to the dtype change.Correction to the analysis in #574 (verified by column dtype + a pandas sandbox): the other
.valuessites listed there (label_storecolumns at ~728/~1325/~1368/~1518/~3232) use integer columns, whose.valuesstays a numpy array and passesset_indextoday — they are not currently broken. This PR fixes the two sites that actually raise; the integer-column sites are left untouched to keep the diff minimal (they could be unified cosmetically later if preferred).Verified in a pandas sandbox (
future.infer_string = True):df.set_index(df["str_col"].values)→TypeError(both sites' pattern)df.set_index("str_col")→ OKFixes #574.