Skip to content

Issue264 - Fixing non-standard shuffle/filter order operations - #265

Merged
bnlawrence merged 3 commits into
mainfrom
issue264
Sep 3, 2026
Merged

Issue264 - Fixing non-standard shuffle/filter order operations#265
bnlawrence merged 3 commits into
mainfrom
issue264

Conversation

@bnlawrence

@bnlawrencebnlawrence commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Description

This pull request handles non-standard shuffle/filter operations. While the original code did things in the right order, it turns out that different orders mean that the shuffle step is seeing a different length buffer. In this situation the chunk buffer reaching the shuffle-undo step isn't always an exact multiple of itemsize. The HDF5 C library handles this by shuffling only the largest itemsize-aligned prefix and passing any trailing remainder bytes through unchanged, which is now what we do.

Closes#264

Checklist

  • This pull request has a descriptive title and labels
  • This pull request has a minimal description (most was discussed in the issue, but a two-liner description is still desirable)
  • Unit tests have been added (if codecov test fails)
  • Any changed dependencies have been added or removed correctly (if need be)
  • If you are working on the documentation, please ensure the current build passes
  • All tests pass

@bnlawrence
bnlawrence marked this pull request as ready for review September 3, 2026 09:58
@codecov

codecovBot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.64%. Comparing base (fc523da) to head (fc753f9).

Additional details and impacted files
@@ Coverage Diff @@## main #265 +/- ##
==========================================
+ Coverage 78.62% 78.64% +0.01% 
==========================================
Files 15 15 Lines 3485 3488 +3 Branches 555 555 ==========================================
+ Hits 2740 2743 +3 
Misses 602 602 Partials 143 143 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

@davidhasselldavidhassell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. I confirm that the new test fails without this PR, and passes with it.

The new line in btree.py:

chunk_buffer=bytes(unshuffled_buffer) +tail

looked a bit odd (why convert the bytarray to bytes)? But it all make sense now that I've looked at it a bit. Whilst you could do something like chunk_buffer = unshuffled_buffer; chunk_buffer.extend(bytarray(tail), that could have unforeseen problems downstream (things expecting bytes rather than bytearray), and any performance improvement is theoretical at best (given that the chunk_buffer will end up in a np.frombuffer call anyway at some future time).

@bnlawrence
bnlawrence merged commit 8550455 into mainSep 3, 2026
8 checks passed
@bnlawrence
bnlawrence deleted the issue264 branch September 3, 2026 11:51
Sign up for freeto 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.

Cannot read SWIFT files due to filter pipeline

2 participants

@bnlawrence@davidhassell