Skip to content

[BUG]: Fix reloaded sphere crashing due to chunking - #5581

Merged
chrishavlin merged 4 commits into
yt-project:mainfrom
jp-duminy:jp-dev
Oct 9, 2026
Merged

chrishavlin merged 4 commits into
yt-project:mainfrom
jp-duminy:jp-dev

Conversation

@jp-duminy

Copy link
Copy Markdown
Contributor

PR Summary

This PR fixes a discovered issue wherein a reloaded sphere dataset produces a TypeError if the sphere is chunked. This arises from an oversight in the chunking implementation: an applied boolean mask was sized to a chunk, whereas the dataset it was applied to was sized to the original file. This therefore produces an incompatible shape error from fancy indexing. The fix is minor and I have added comments to explicitly point out the different file shapes to hopefully prevent such an oversight occurring in future.

The second commit of the PR adds a small unit test to check this behaviour. It errors on the current yt version, but now passes following the changes. As part of this test we also demand the reloaded sphere dataset reproduces the data before it was saved; this also passes.

The issue was first described in #5576.

AI Disclosure

I used AI for (leaving both boxes unchecked certifies that no AI was used in these changes):

  • low level assistance: code-completion, syntax lookup, explaining existing code.
  • substantive generation: substantive portions of code and/or documentation were written by AI then edited or submitted as is. This includes copying code from an AI chat or agentic coding. I have read and can explain every changed line.

I used the following AI tools:

Claude (Anthropic), Claude Opus 5.5

The source code modification in the first commit is written by me; I had Claude take a look to catch anything I might have missed. The corresponding test was then written by me.

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.
  • AI Disclosure completed if AI tools were used.

…ts in frontends/ytdata/io.py on _read_particle_data_file(); added comments to prevent this bug occurring again.
…he same data to what was saved; that the saved data is identical regardless of how accessed; and that chunking does not crash (in response to bug fixed in a2e6cfe)
@welcome

welcome Bot commented Oct 9, 2026

Copy link
Copy Markdown

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

@jp-duminy

Copy link
Copy Markdown
Contributor Author

@brittonsmith a fun side quest on the project.

@chrishavlin
chrishavlin self-requested a review October 9, 2026 14:04
@chrishavlin chrishavlin added this to the 4.4.3 milestone Oct 9, 2026
@chrishavlin chrishavlin added the bug label Oct 9, 2026
@chrishavlin chrishavlin linked an issue Oct 9, 2026 that may be closed by this pull request
@chrishavlin

chrishavlin commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

thanks for the PR @jp-duminy ! the failing test is unrelated and I think will be fixed by a merge with main

@chrishavlin

Copy link
Copy Markdown
Contributor

@jp-duminy just did the merge for you and pushed it up FYI.

@chrishavlin chrishavlin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change looks good, but the test needs an update (see comments). Note, pull the branch locally before making changes since I merged your branch with main for you.

Comment thread yt/frontends/ytdata/tests/test_data_reload.py Outdated
…ct a subset of the domain, thus appropriately reflecting the typical use case scenario.

@chrishavlin chrishavlin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

assuming tests pass again, this is good to go! Thanks for the issue and fix @jp-duminy !

@chrishavlin
chrishavlin enabled auto-merge October 9, 2026 19:45
@chrishavlin
chrishavlin merged commit f8d7975 into yt-project:main Oct 9, 2026
13 checks passed
@welcome

welcome Bot commented Oct 9, 2026

Copy link
Copy Markdown

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆

chrishavlin added a commit that referenced this pull request Oct 9, 2026
…1-on-yt-4.4.x

Backport PR #5581 on branch yt-4.4.x ([BUG]: Fix reloaded sphere crashing due to chunking)
@jp-duminy
jp-duminy deleted the jp-dev branch October 10, 2026 10:00
@jp-duminy

Copy link
Copy Markdown
Contributor Author

@chrishavlin thank you for walking me through this one, and I'm glad I could contribute. Have a lovely weekend.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Querying a reloaded sphere dataset fails when chunked

2 participants