Skip to content
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

fix: set dtype in full_like #2251

Merged
merged 6 commits into from
Feb 16, 2023
Merged

fix: set dtype in full_like #2251

merged 6 commits into from
Feb 16, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Feb 16, 2023

This PR directly sets dtype in the nplike.full_like call, which I think is all we need to do here.

Fixes #2250

@agoose77
Copy link
Collaborator Author

@jpivarski I also added support for EmptyArray if a dtype is passed. Unlike ak.values_astype, I didn't add a including_unknown flag because this seems like a bugfix and a sensible choice. Is there a reason that we added including_unknown to values_astype besides backwards compatibility? Would you like me to do the same here?

@agoose77 agoose77 marked this pull request as draft February 16, 2023 19:37
@agoose77 agoose77 marked this pull request as ready for review February 16, 2023 19:41
@agoose77 agoose77 marked this pull request as draft February 16, 2023 19:41
@agoose77 agoose77 temporarily deployed to docs-preview February 16, 2023 19:53 — with GitHub Actions Inactive
@jpivarski
Copy link
Member

The reason is that sometimes an unknown type fills in for a NumpyArray, and sometimes it fills in for something else.

In a situation like this:

>>> partition_1 = ak.from_json("[[1.1, 2.2], [3.3]]")
>>> partition_1.type.show()
2 * var * float64
>>> 
>>> partition_2 = ak.from_json("[[]]")
>>> partition_2.type.show()
1 * var * unknown
>>> 
>>> partition_3 = ak.from_json("[]")
>>> partition_3.type.show()
0 * unknown

you wouldn't want to fill in the unknown types as a byproduct of turning the floats into integers or something. You'd want these unknowns to stay unknown until concatenation.

But there are other situations in which you want unknown to become numeric, like np.array([]).

So I think it would be a good idea for both values_astype and full_like (with dtype) to have a boolean including_unknown argument.

@agoose77
Copy link
Collaborator Author

But there are other situations in which you want unknown to become numeric

That's a good point, and links to our conversations yesterday!

@agoose77 agoose77 marked this pull request as ready for review February 16, 2023 21:59
@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #2251 (7aebbaa) into main (dc8f334) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/operations/ak_full_like.py 98.27% <100.00%> (+0.09%) ⬆️
src/awkward/operations/ak_ones_like.py 91.66% <100.00%> (ø)
src/awkward/operations/ak_zeros_like.py 93.33% <100.00%> (ø)
src/awkward/operations/ak_strings_astype.py 96.15% <0.00%> (-3.85%) ⬇️
src/awkward/contents/unionarray.py 83.44% <0.00%> (-0.68%) ⬇️

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I looked over this when it was in the draft stage, and just did again now. I think it's great and ready to be merged!

@agoose77 agoose77 temporarily deployed to docs-preview February 16, 2023 22:12 — with GitHub Actions Inactive
@agoose77 agoose77 merged commit 7b220ee into main Feb 16, 2023
@agoose77 agoose77 deleted the agoose77/fix-full-like-bool branch February 16, 2023 22:25
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.

ak.full_like does not fill with correct value when dtype is changed
2 participants