Skip to content

feat: improve and fix delayed length calculations and add delayed length calculation for layouts that need it#3889

Open
ikrommyd wants to merge 19 commits intoscikit-hep:mainfrom
ikrommyd:fix-length-calculations
Open

feat: improve and fix delayed length calculations and add delayed length calculation for layouts that need it#3889
ikrommyd wants to merge 19 commits intoscikit-hep:mainfrom
ikrommyd:fix-length-calculations

Conversation

@ikrommyd
Copy link
Collaborator

Partial fix for #3888
Tackles Bug 1 only

Comment on lines -180 to +181
and mask.length is not unknown_length
and length > content.length * 8
and ak._util.maybe_length_of(content) is not unknown_length
and length > content.length
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is actually funny. It's always been testing the wrong thing given the error message it wants to raise

@ikrommyd ikrommyd requested a review from pfackeldey February 19, 2026 19:46
@github-actions
Copy link

The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3889

@codecov
Copy link

codecov bot commented Feb 19, 2026

Codecov Report

❌ Patch coverage is 85.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.66%. Comparing base (a6c3b25) to head (060c3d2).

Files with missing lines Patch % Lines
src/awkward/contents/bitmaskedarray.py 75.00% 2 Missing ⚠️
src/awkward/operations/ak_from_buffers.py 83.33% 2 Missing ⚠️
src/awkward/contents/recordarray.py 88.88% 1 Missing ⚠️
src/awkward/contents/regulararray.py 90.90% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/awkward/contents/bytemaskedarray.py 88.40% <ø> (ø)
src/awkward/contents/unionarray.py 86.56% <ø> (ø)
src/awkward/contents/recordarray.py 85.58% <88.88%> (+0.45%) ⬆️
src/awkward/contents/regulararray.py 87.19% <90.90%> (+0.15%) ⬆️
src/awkward/contents/bitmaskedarray.py 70.14% <75.00%> (+0.11%) ⬆️
src/awkward/operations/ak_from_buffers.py 92.73% <83.33%> (-0.66%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ikrommyd ikrommyd changed the title fix: forgotten delayed length calculations fix: add forgotten delayed length calculations Feb 19, 2026
@ikrommyd ikrommyd changed the title fix: add forgotten delayed length calculations fix: fix and improve delayed length calculations Feb 20, 2026
@ikrommyd ikrommyd changed the title fix: fix and improve delayed length calculations feat: improve and fix delayed length calculations and add delayed length calculation for layouts that need it Feb 20, 2026
@ikrommyd ikrommyd marked this pull request as draft February 20, 2026 16:14
@ikrommyd ikrommyd marked this pull request as ready for review February 23, 2026 21:01
Comment on lines -180 to -181
and mask.length is not unknown_length
and length > content.length * 8
Copy link
Member

Choose a reason for hiding this comment

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

content.length usually measures bytes, while the length variable might represent the expected size in bits.

Copy link
Collaborator Author

@ikrommyd ikrommyd Feb 24, 2026

Choose a reason for hiding this comment

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

This is the whole snippet (without my changes)

        if (
            content.backend.nplike.known_data
            and length is not unknown_length
            and mask.length is not unknown_length
            and length > content.length * 8 
        ):
            raise ValueError(
                f"{type(self).__name__} 'length' ({length}) must be <= len(content) ({content.length})"
            )

So I think this was meant to check whether length > content.length. The length of the mask is checked right above for length > mask.length * 8. Only mask.length is in bits as far as I know/can understand. .length is always the "physical" length of the layout/array. So length and content.length are neither in bytes nor bits in general. It's the "number of elements" (which does correspond to bytes of the mask in this case).

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