Skip to content

FIX Preserve defaultdicts in to_device - #1153

Closed
aswanth-07 wants to merge 1 commit into
skorch-dev:masterfrom
aswanth-07:fix/to-device-defaultdict
Closed

FIX Preserve defaultdicts in to_device#1153
aswanth-07 wants to merge 1 commit into
skorch-dev:masterfrom
aswanth-07:fix/to-device-defaultdict

Conversation

@aswanth-07

Copy link
Copy Markdown

Summary

  • preserve collections.defaultdict instances when recursively moving mapping values
  • carry over the original default_factory
  • add regression coverage and a changelog entry

Problem

to_device supports mapping inputs and reconstructs them with type(X)(mapping). For a defaultdict, the first constructor argument is the default factory, so the public utility currently raises:

TypeError: first argument must be callable or None

This change uses the defaultdict(default_factory, mapping) constructor while retaining the existing behavior for other mapping types.

Tests

  • python -m pytest skorch/tests/test_utils.py -q -k TestToDevice (13 passed, 17 skipped)
  • python -m pytest skorch/tests/test_utils.py -q (105 passed, 22 skipped)
  • git diff --check

Full-file Flake8/Pylint checks report pre-existing findings in these files; the changed lines introduce no Flake8 diagnostics.

AI assistance

This pull request was prepared with Codex assistance. I reviewed the reproduced failure, the focused implementation, and the test results before submission.

@aswanth-07
aswanth-07 marked this pull request as ready for review August 9, 2026 16:41
@BenjaminBossan

Copy link
Copy Markdown
Collaborator

Thanks for the PR. This sounds like an extreme edge case. Did you run into this issue yourself or is this a pro-active fix?

@aswanth-07

Copy link
Copy Markdown
Author

Thanks for the PR. This sounds like an extreme edge case. Did you run into this issue yourself or is this a pro-active fix?

Thanks! This was a proactive fix rather than something I encountered in an application. While reviewing to_device's handling of Mapping subclasses, I noticed that reconstructing mappings with type(X)(mapping) is incompatible with defaultdict, since its first positional argument is the default factory.
I reproduced the failure locally with a minimal defaultdict example and added the regression test before making the change. Codex assisted with the repository review and implementation, as disclosed in the PR, and I verified the behavior and tests locally.

@aswanth-07
aswanth-07 force-pushed the fix/to-device-defaultdict branch from b2c8f4d to 389df27 Compare August 11, 2026 08:14
@BenjaminBossan

Copy link
Copy Markdown
Collaborator

Thanks for clarifying. I think it's very unlikely that defaultdicts would be used here, that has never come up so far. Adding the extra complexity for a purely theoretical use case is not worth it. Still, thanks for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants