Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Fix calls to np.ma.masked_where#20511
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
For the same reason as Quiver inmatplotlib#4250.
| defautoscale(self,A): | ||
| # docstring inherited. | ||
| super().autoscale(np.ma.masked_less_equal(A,0,copy=False)) | ||
| super().autoscale(np.ma.array(A,mask=(A<=0))) |
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.
Does this save the copy step? It seems like this is creating a new array anyways, so I'm wondering if it is gaining anything relative to just removing thecopy=False?
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.
The default fornp.ma.array iscopy=False, keep_mask=True. The former will not copy the data, and the latter will use the existing mask (if any) with the new mask, which will make a copy. Removingcopy=False frommasked_less_equal makes a copy of both.
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.
Thanks for clarifying! I was curious how to test if this actually holds and learned aboutnp.shares_memory today. I did verify this assertion with that.
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.
You can also check, if you haveb = np.ma.array(a), thatb.base is a (ifa is originally a plain array), orb.base is a.base (ifa is originally a masked array).
Uh oh!
There was an error while loading.Please reload this page.
NumPy 1.21.0 fixed a bug with `np.ma.masked_where` where `copy=False`now modifies the input if it is masked, which we do not want to do.`np.ma.array` defaults to not copying the data, but copies the mask(adding the new mask), which is what we wanted with `copy=False`.
…511-on-v3.4.xBackport PR#20511 on branch v3.4.x (Fix calls to np.ma.masked_where)
jorisvandenbossche commentedAug 21, 2021
@QuLogic a small note that Ithink this change can cause a bug when the array We ran into this with GeoPandas (geopandas/geopandas#2066, didn't test it, but I assume this PR is the cause), where we were (incorrectly) setting the array of a color map with a list Just noting it here, I'll let it to your judgement whether this is worth addressing or not (it's certainly not needed just for GeoPandas). |
greglucas commentedAug 21, 2021
Just a note that I am pretty sure this is fixed in master due to a new call to a call to matplotlib/lib/matplotlib/cm.py Line 459 inca275dc
v3.4.3 >>>sm.set_array([])>>>sm.get_array()[] master >>>sm.set_array([])>>>sm.get_array()masked_array(data=[],mask=False,fill_value=1e+20,dtype=float64) I don't know if another 3.4.x release is planned sometime, but it should be a pretty simple fix to add that call in to cast to array immediately in the set_array call. |
QuLogic commentedAug 23, 2021
That came in from#18870, which I don't think we can backport fully. But we can make a small correction to this, as it's not required that |
PR Summary
Due tonumpy/numpy#18967, I've audited all our calls to
np.ma.masked_*(copy=False). When the input comes directly from the user, or is already stored somewhere already, then we don't want these calls to modify the existing values.This affects
np.ma.masked_whereand allsimple wrappers around it likemasked_less_equal. It doesnot affectmasked_invalid, which continues to not modify the input. I have asked upstreamnumpy/numpy#19332 whether that is intended. If they intend to changemasked_invalidalso, then we will need to make a few more changes like this, but we are mostly okay.PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).