Skip to content

fix: warn on implicit zero-fill and preserve empty dicts in Batch - #1296

Open
Lidang-Jiang wants to merge 5 commits into
thu-ml:masterfrom
Lidang-Jiang:fix/batch-none-and-empty-handling
Open

fix: warn on implicit zero-fill and preserve empty dicts in Batch#1296
Lidang-Jiang wants to merge 5 commits into
thu-ml:masterfrom
Lidang-Jiang:fix/batch-none-and-empty-handling

Conversation

@Lidang-Jiang

Copy link
Copy Markdown

Summary

Addresses #1088 and #1089:

The fix is backward compatible — the zero-fill behavior itself is unchanged (changing it would break existing code), but users are now explicitly warned when it happens.

Design decisions

  • Extracted _validate_and_convert_batches() and _warn_numeric_zero_fill() as module-level helpers to keep stack_() complexity under ruff's C901 limit (max 20).
  • Warning is UserWarning (not DeprecationWarning) since the zero-fill behavior is not being deprecated, only made visible.
Before (bug reproduction)
--- Bug #1089: Empty dict silently dropped ---
Input: [{"a": 1}, {}]
Result: Batch(
    info: Batch(
              a: array([1]),   # length 1 — index [1] lost
          ),
)

--- Bug #1088: None replaced by 0 silently ---
Batch(a=[1, 2, 3], b=[{"c": 1}, {}, {"c": 3}])
# b.c = array([1, 0, 3]) — 0 silently inserted at index [1], no warning
After (with fix)
--- Bug #1089: Empty dict now preserved ---
Input: [{"a": 1}, {}]
Result: Batch(
    info: Batch(
              a: array([1, 0]),   # length 2 — both indices preserved
          ),
)
Length: 2

--- Bug #1088: Warning on 0 fill ---
UserWarning: Key 'c' is not present in all batches during stacking
(missing at indices [1]). Filling missing entries with 0 for numeric
type (ndarray), which may mask truly missing values. Consider using
None or np.nan to represent missing data explicitly.
Test results (75 passed)
test/base/test_batch.py ......................................... [ 82%]
test/base/test_batch.py::TestBatchNoneAndEmptyHandling .......... [100%]
======================== 75 passed, 14 warnings in 2.31s ========================

Test plan

  • 13 new tests in TestBatchNoneAndEmptyHandling covering empty dict preservation and warning behavior
  • All 75 existing batch tests pass
  • ruff check and black pass

Addresses thu-ml#1088 and thu-ml#1089:
- Issue thu-ml#1089: Empty dicts in batch lists were silently dropped during
  stack_, causing index position misalignment. Now all entries are
  preserved in the batch_list.
- Issue thu-ml#1088: When __setitem__ or stack_ fills missing numeric keys
  with 0, a UserWarning is now emitted to alert users that the zero
  value may mask truly missing data.

Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ddce36a42e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread tianshou/data/batch.py
has_any_nonempty = False
for batch in batches:
if isinstance(batch, dict):
batch_list.append(Batch(batch) if len(batch) > 0 else Batch())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reconcile nested schemas before keeping empty stack entries

Keeping top-level empty dicts as Batch() placeholders causes mixed nested schemas to fail in cases that previously worked. For example, Batch.stack([{"info": {"a": 1}}, {}, {"info": {"b": 2}}]) now routes info through the partial-key assignment path; the first value allocates only subkey a, and assigning the third value then raises ValueError("Creating keys is not supported by item assignment.") when subkey b appears. This breaks stacking whenever an empty entry sits between batches whose nested keys differ.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. When empty dicts are preserved between batches whose nested keys differ, the element-by-element assignment path fails because __setitem__ doesn't support creating new keys.

Fixed in 0953587: for partial keys containing nested Batch/dict values, we now collect all entries and delegate to recursive Batch.stack instead of assigning one-by-one. This handles differing nested schemas correctly.

Verified locally:

>>> Batch.stack([{'info': {'a': 1}}, {}, {'info': {'b': 2}}])
Batch(info: Batch(a: array([1, 0, 0]), b: array([0, 0, 2])))

- Remove stale type: ignore comments that mypy now flags as errors
- Remove associated TODO comment since ignores are no longer needed
…tive

The for-loop variable `value` from the shared-keys block retained type
`list[Any]` in mypy's analysis, causing the later `isinstance(value, Batch)`
check to be flagged as unreachable. Rename to `val` to give mypy a fresh
binding.

Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
When empty dicts are preserved between batches whose nested keys
differ (e.g. stack([{"info": {"a": 1}}, {}, {"info": {"b": 2}}])),
the previous element-by-element assignment would fail with
ValueError("Creating keys is not supported by item assignment.")
because the first value allocated only subkey 'a', and the third
value tried to create subkey 'b'.

Fix by detecting nested Batch/dict values in partial keys and using
recursive Batch.stack instead of element-by-element assignment.

Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
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.

1 participant