pynwb icon indicating copy to clipboard operation
pynwb copied to clipboard

Fix pandas FutureWarning: Avoid chained assignment with copy() in icephys.py

Open Copilot opened this issue 4 months ago • 5 comments

This PR fixes pandas FutureWarning messages that appear when running PyNWB tests with Python 3.14. The warnings indicate that chained assignment patterns will not work in pandas 3.0 with Copy-on-Write enabled.

Problem

When running tests with Python 3.14.0rc2+, the following warnings were raised:

FutureWarning: ChainedAssignmentError: behaviour will change in pandas 3.0!
You are setting values through chained assignment. Currently this works in certain cases, but when using Copy-on-Write (which will become the default behaviour in pandas 3.0) this will never work to update the original DataFrame or Series, because the intermediate object on which we are setting values will behave as a copy.

Use `df.loc[row_indexer, "col"] = values` instead, to perform the assignment in a single step and ensure this keeps updating the original `df`.

The warnings occurred in src/pynwb/icephys.py at lines 749, 751, and 756 in the IntracellularRecordingsTable.to_dataframe() method.

Solution

The issue was resolved by copying DataFrame columns before performing transformations, which avoids the chained assignment pattern that triggers pandas warnings:

Before:

res[('electrodes', 'electrode')] = [e.object_id for e in res[('electrodes', 'electrode')]]
res[('stimuli', 'stimulus')] = [...]
res[('responses', 'response')] = [...]

After:

electrode_col = res[('electrodes', 'electrode')].copy()
res[('electrodes', 'electrode')] = [e.object_id for e in electrode_col]
# Similar pattern for stimuli and responses

This approach was chosen over the initially attempted .loc syntax because:

  1. The DataFrame has a MultiIndex column structure with tuples like ('electrodes', 'electrode')
  2. Using .loc[:, ('column', 'name')] = [...] caused a ValueError: Must have equal len keys and value when setting with an ndarray
  3. The .copy() approach maintains identical functionality while avoiding chained assignment

Changes

  • Line 749: Added electrode_col = res[('electrodes', 'electrode')].copy() before assignment
  • Line 751: Added stimulus_col = res[('stimuli', 'stimulus')].copy() before assignment
  • Line 756: Added response_col = res[('responses', 'response')].copy() before assignment

The fix is minimal and preserves all existing functionality while ensuring compatibility with future pandas versions.

Fixes #2126.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot avatar Aug 19 '25 22:08 Copilot

@rly 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs.

I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review.

Copilot avatar Aug 19 '25 22:08 Copilot

@copilot This fix may ultimately require a PR in the HDMF repository instead of PyNWB.

rly avatar Aug 19 '25 22:08 rly

@copilot please review and address the failing tests

rly avatar Aug 20 '25 23:08 rly

@copilot This fix may ultimately require a PR in the HDMF repository instead of PyNWB.

You're correct. After investigating, the issue occurs in PyNWB's IntracellularRecordingsTable.to_dataframe() method where it modifies the DataFrame returned by HDMF's AlignedDynamicTable.to_dataframe(). The chained assignment pattern in PyNWB is triggering the pandas FutureWarning when modifying columns with MultiIndex structure. The fix would be more appropriate in PyNWB, but requires a different approach than simple .loc syntax due to the DataFrame structure. Reverting my changes for now.

@copilot please review and address the failing tests

I've identified and fixed the issue causing the test failures. The problem was that my initial .loc syntax approach caused a pandas ValueError due to the MultiIndex DataFrame structure. I've now implemented a proper fix using .copy() to avoid chained assignment warnings while preserving the original functionality. Commit e7c208d addresses the failing tests.

[!WARNING]

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • http://github.com:80/NeurodataWithoutBorders/nwb-schema.git/info/refs
    • Triggering command: /usr/lib/git-core/git-remote-http origin REDACTED (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot avatar Aug 20 '25 23:08 Copilot

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 95.19%. Comparing base (387f80b) to head (e7c208d). :warning: Report is 7 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #2129   +/-   ##
=======================================
  Coverage   95.19%   95.19%           
=======================================
  Files          28       28           
  Lines        2849     2852    +3     
  Branches      736      736           
=======================================
+ Hits         2712     2715    +3     
  Misses         83       83           
  Partials       54       54           
Flag Coverage Δ
integration 74.08% <0.00%> (-0.08%) :arrow_down:
unit 85.09% <100.00%> (+0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Aug 20 '25 23:08 codecov[bot]