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

ENH: allow setting mask attribute? #42

Open
mdhaber opened this issue Dec 12, 2024 · 4 comments
Open

ENH: allow setting mask attribute? #42

mdhaber opened this issue Dec 12, 2024 · 4 comments

Comments

@mdhaber
Copy link
Owner

mdhaber commented Dec 12, 2024

ISTM it would be safe to allow setting the mask by taking the logical or of the original mask and the provided mask. Users might find this convenient rather than only being able to set the mask with mxp.asarray. Thoughts?

@seberg
Copy link
Collaborator

seberg commented Dec 12, 2024

You try to support packages that don't support assignment to the mask already I think.

I would suggest to resist this temptation, it seems like a way to creatn confusion via strange mutability for little gain.
Instead, make sure that getting a copy/view with a different mask is easy?

(view semantics may not make sense for all libraries of course)

@mdhaber
Copy link
Owner Author

mdhaber commented Dec 12, 2024

You try to support packages that don't support assignment to the mask already I think.

Packages that support mutation wouldn't support mutation. I wouldn't try to change that.

In any case, I didn't mean setting elements of the mask attribute; I meant reassignment. It would either set the final mask to mask_old | mask_new or raise if that array is different from mask_new.

I would suggest to resist this temptation

It's not a temptation I need to resist. It's less work if I just leave things along, so I'm not tempted.

Instead, make sure that getting a copy/view with a different mask is easy?

It's never more than a single line away.

mxp.asarray(old.data, mask=new_mask)

but this doesn't (can't) protect against unmasking previously masked elements whereas

old.mask = new_mask

could.

@seberg
Copy link
Collaborator

seberg commented Dec 12, 2024

:), sorry if that was a bit strong reaction. I do feel I have heard of contexts where you create different masks for the same data array to do different analyses.
But maybe methods like old.apply_mask() or so can achieve e.g. the | behavior, and for when everything is unmasked originally, the asarray call seems probably good enough?

Overall, arr.mask = new_mask just seems maybe slightly off and like it has the potential to create issues down the line (I would not have expected it to do mask_old | mask_new).

@mdhaber
Copy link
Owner Author

mdhaber commented Dec 12, 2024

OK, but as much as possible, I'm avoiding creating features that are not specifed by the array API. Having a mask attribute is pretty much a necessity, so I was hoping to work it into that + regular Python, hence the idea of providing a setter for the mask property. Again, it could do the or operation or just raise if the new mask would expose any previously masked elements. (Once an element is masked and an operation is performed, the data in the masked location won't necessarily be meaningful.)

I think I'll wait for there to be users who can weigh in before doing anything.

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

No branches or pull requests

2 participants