-
-
Notifications
You must be signed in to change notification settings - Fork 12k
BUG: allow MaskedArray.fill_value be a string when dtype=StringDType
#29423
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
BUG: allow MaskedArray.fill_value be a string when dtype=StringDType
#29423
Conversation
This fix adds the StringDType data type character code 'T' to the list of accepted data types that can use a 'fill_value' as string. Currently just accepts None. "See numpy#29421" Issue.
MaskedArray.fill_value be a string when dtype=StringDType
|
Sorry - it's not totally clear to me what this fixes. Can you add a test? Maybe there's an existing test for this in the masked array tests that check the other dtypes you can extend. |
|
It looks like if I just extend the existing tests I can trigger the bug this PR is fixing. See ngoldbaum@4343115. You can pull that commit and push or I can just push directly to your PR branch, whichever you feel most comfortable with. Generally I don't push to people's PR branches without explicit permission. Once there are tests I'm happy to merge this. |
|
Oh I see now, my apologies Nathan, as a first time contributor I totally miss the testing inclusion with numpy development standards. I think I'll do the pull and push, but let me double check this just to make sure I'm understanding this correctly. Thank again. |
numpy/ma/tests/test_core.py
Outdated
| ma2 = masked_array(["cde", "b", "a"], mask=[0, 1, 0], fill_value=fill, dtype=dt) | ||
| assert_equal(op(ma1, ma2)._data, op(ma1._data, ma2._data)) | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to appease the linter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding test functions for test_core.py now.
|
Not ready to merge yet, I'm double checking something I just noticed. |
Still double-checking? |
|
I was checking the behavior of Lines 176 to 185 in fda7b4c
I was wondering about adding one for StringDType like N/A as for string. As it is now still works if MaskedArray initializes without fill_value set, a fill_value is given regardless, just like for dtype=object O.do you think I should add 'T' : 'N/A' or 'T' : b'N/A' to the default_filler dict?
|
|
I think using "N/A" is fine. It might make sense eventually to also do something sensible if someone wants to use a fill value that isn't a string, but we can handle that then. You can use but "N/A" is probably a fine default to use. If you wanted to make it so non-string fill values work, you'll need to set |
|
OK, awesome. It’s borderline but IMO this deserves a release note too. See https://github.com/numpy/numpy/blob/main/doc/release/upcoming_changes/README.rst and the other release note fragments in that directory for instructions and examples. |
|
Oops, looks like I forgot to come back to this. I'll try to give this another once-over and hopefully hit the merge button later today. |
|
Let me know if there anything else you need me to do @ngoldbaum |
|
Thanks for your patience @phdparedes and for the contribution! Hope to see your github handle again :) |
…pe` (numpy#29423) * BUG: allow ma.MaskedArray.fill_value be a string when dtype=StringDType This fix adds the StringDType data type character code 'T' to the list of accepted data types that can use a 'fill_value' as string. Currently just accepts None. "See numpy#29421" Issue. * TST: add tests for StringDType MaskedArray use * TST: add tests for fill_value in StringDType MaskedArray * TST: fix format in the test functions added * Add default fill value 'N/A' for StringDType * TST: Add tests for StringDType masked array default fill and slicing * DOC: add release note for StringDType fill_value support (numpygh-29423) --------- Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
|
hi folks, cheers for this - @ngoldbaum this has a side-effect that I'm not sure if it's a bug or a feature: previously a |
This pull request includes a minor fix in the
_check_fill_valuefunction innumpy/ma/core.py. The change adjusts the condition to include the character'T'in the type check forndtype.char.numpy/ma/core.py: Updated the conditional check in_check_fill_valueto include'T'in the list of validndtype.charvalues, ensuring compatibility with additional data types.This ensures that
MaskedArraywithdtype=StringDTypeand withfill_valueset to a string value initializes, and also whenfill_valueis reassigned."See BUG: Error in
MaskedArraywithStringDTypewhen settingfill_value#29421" Issue.