Skip to content

fix: proposal for #2947#3191

Merged
agoose77 merged 3 commits into
scikit-hep:mainfrom
maxgalli:fix_2947
Jul 31, 2024
Merged

fix: proposal for #2947#3191
agoose77 merged 3 commits into
scikit-hep:mainfrom
maxgalli:fix_2947

Conversation

@maxgalli

Copy link
Copy Markdown
Collaborator

Test against np.integer as well in ArrayModuleNumpyLike.shape_item_as_index such that it does not break if ak.from_buffers is built with a numpy integer instead of an integer.
I have the feeling that this might be an "easy fix" and that maybe the fix should be implemented at a deeper level - please let me know if this is the case.

@codecov

codecov Bot commented Jul 30, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.21%. Comparing base (b749e49) to head (65fd4eb).
Report is 129 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
src/awkward/operations/ak_from_buffers.py 94.19% <100.00%> (+0.07%) ⬆️

... and 100 files with indirect coverage changes

@maxgalli maxgalli changed the title Propose fix for https://github.com/scikit-hep/awkward/issues/2947 fix: proposal for #2947 Jul 31, 2024
@agoose77

Copy link
Copy Markdown
Collaborator

@maxgalli thanks for this PR! I think you're right that we'd prefer to tackle this slightly differently; presently, our internal representation of shape items is int or our unknown length types. So, we'd prefer to cast the NumPy types in the entry-points to our internals, e.g. ak.from_buffers.

@maxgalli

Copy link
Copy Markdown
Collaborator Author

@maxgalli thanks for this PR! I think you're right that we'd prefer to tackle this slightly differently; presently, our internal representation of shape items is int or our unknown length types. So, we'd prefer to cast the NumPy types in the entry-points to our internals, e.g. ak.from_buffers.

thanks @agoose77, I changed the PR accordingly to perform the casr in ak.from_buffers. Are there other functions in which this might be needed?

@agoose77

Copy link
Copy Markdown
Collaborator

At this stage, any function that accepts a length e.g. ak.unflatten, ak.from_buffers, and maybe some of the functions like ak.pad_none etc. I think it's reasonable to wait for bug reports there, rather than guessing :)

@agoose77

Copy link
Copy Markdown
Collaborator

This LGTM, and I feel confident in merging despite being out of the regular development cycle. @jpivarski I'm notifying you that I'm merging despite feeling comfortable to do so!

@agoose77

Copy link
Copy Markdown
Collaborator

@maxgalli thanks for this useful PR! <3

@agoose77
agoose77 merged commit d5bc304 into scikit-hep:main Jul 31, 2024
pfackeldey pushed a commit to pfackeldey/awkward that referenced this pull request Jul 31, 2024
* fix: cast numpy integers to integers in ak.from_buffers (fixes scikit-hep#2947)

* Add test for 3502225e9246ce0197d6143c9d94c9e1aeec88b8

* style: pre-commit fixes

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@jpivarski

Copy link
Copy Markdown
Member

@all-contributors please add @maxgalli for code

@allcontributors

Copy link
Copy Markdown
Contributor

@jpivarski

I've put up a pull request to add @maxgalli! 🎉

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.

3 participants