Skip to content

Add bool to allowed conversion between numpy arrays and RVec #18087

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 11, 2025

Conversation

Aditya-138-12
Copy link
Contributor

@Aditya-138-12 Aditya-138-12 commented Mar 21, 2025

This Pull request:

Add support for NumPy boolean type 'b1' in RVec conversion

Changes or fixes:

Modify _get_cpp_type_from_numpy_type function in _rvec.py to map NumPy's 'b1', thereby adding this in the dict cppytypes "b1":"bool"

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #18085

@Aditya-138-12 Aditya-138-12 changed the title [RDF] Update _rvec.py [RDF] Missing bool type _rvec.py Mar 21, 2025
@Aditya-138-12
Copy link
Contributor Author

Hi @vepadulano @dpiparo
Can we also add other types like 8 bit usigned integer i1 ?... so that possibly this kind of bug may not arise in future?

Copy link

github-actions bot commented Mar 21, 2025

Test Results

    18 files      18 suites   4d 10h 56m 31s ⏱️
 2 721 tests  2 721 ✅ 0 💤 0 ❌
47 506 runs  47 506 ✅ 0 💤 0 ❌

Results for commit 1170796.

♻️ This comment has been updated with latest results.

@Aditya-138-12
Copy link
Contributor Author

Aditya-138-12 commented Mar 21, 2025

Hi @vepadulano , I have added 4 more types.

@pcanal pcanal dismissed a stale review March 23, 2025 15:29

Please tell us why you approve this request.

@vepadulano vepadulano changed the title [RDF] Missing bool type _rvec.py [df] Extend Python array data type to RVec data type conversion map Mar 27, 2025
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the issue! I left a minor comment inline, then I believe we need more tests in test/rvec_asrvec.py. The list of dtypes in the AsRVec test case class should be extended to include all the dtypes represented in the map after the changes.

@ferdymercury ferdymercury requested a review from vepadulano March 28, 2025 15:36
@vepadulano vepadulano closed this Apr 5, 2025
@vepadulano vepadulano reopened this Apr 5, 2025
@vepadulano vepadulano changed the title [df] Extend Python array data type to RVec data type conversion map Add bool to allowed conversion between numpy arrays and RVec Apr 9, 2025
Add support for NumPy boolean type 'b1' in RVec conversion

Modify _get_cpp_type_from_numpy_type in _rvec.py to map NumPy's 'b1'
@vepadulano
Copy link
Member

Thank you for reverting back to the original change to fix the related issue @Aditya-138-12! I believe the extension of the type map was a good idea, but the issues that arose from it are unrelated to fixing the original problem and will need more careful attention, to be left for a follow-up PR 👍

@lwpiotr
Copy link

lwpiotr commented Apr 10, 2025

Shouldn't short and unsigned short also be added here?

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Thank you @Aditya-138-12 , LGTM!

@vepadulano
Copy link
Member

Shouldn't short and unsigned short also be added here?

Hi @lwpiotr ! Thanks for showing interest in this PR! As you can see from the previous comments, this was already attempted by the author, but different issues arose which are unrelated to the request from the user at the linked issue. In general, I agree this type conversion map could be further extended, but it is beyond the scope of this PR 👍 Feel free to propose a new PR to address the changes!

@vepadulano vepadulano merged commit 17af842 into root-project:master Apr 11, 2025
21 of 23 checks passed
@Aditya-138-12
Copy link
Contributor Author

Thank you @Aditya-138-12 , LGTM!

@vepadulano Thank you for your time and guidance!!

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.

Missing bool type in creating RDataFrame
4 participants