Fix evaluating colormaps on non-numpy arrays#24009
Conversation
|
Even though we do not have it as a test dependency, it may still make sense to add a test for it (e.g. add a pytorch fixture). There are some plans to run a weekly test with "all" possible dependencies, although not implemented yet. |
jklymak
left a comment
There was a problem hiding this comment.
This change makes good sense to me; we should not be using numpy methods on objects until we are sure they are arrays.
Not sure about the test. Definitely we don't want to depend on PyTorch. Mocking up a class that errors on np.isnan would be ideal, but this is clear enough to probably do without.
|
I wouldn't want to add a mock test here. That'd be rather self-fulfilling, and we would not have added such a test if we had written that part of the code from scratch. The only thing, I could imagine would be a real pytorch test with |
The point of such tests is to not break again in a refactor. All the dances around the array conversion are delicate, and documenting behaviour with tests is idea. |
|
I'm not able to come up with a suitable mock object. |
Closes matplotlib#23132. I can't specifically test that case, because we don't have pytorch as a test dependency. However, I'd claim that deferring np.nan() to after converting to a numpy array is obviously the right thing to do.
|
Rebased to fix CI. |
|
I could go either way on the backport. |
…009-on-v3.6.x Backport PR #24009 on branch v3.6.x (Fix evaluating colormaps on non-numpy arrays)
Closes #23132.
I've tested locally that this fixes the issue. I can't add a testcase for this specifically, because we don't have pytorch as a test dependency. However, I'd claim that deferring
np.nan()to after converting to a numpy array is obviously the right thing to do. - Nobody knows what can happen when applyingnp.nan()to an arbitrary object, but luckly, we have the converted array just one line later and can work with that.