Repository navigation
Fix evaluating colormaps on non-numpy arrays - #24009
tacaswell merged 1 commit into
Conversation
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
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 torch = pytest.import_or_skip('torch'). Such a test would currently be inactive, but would be picked up if we add a weekly all-dependencies test. Not sure if it's worth though. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
I'm not able to come up with a suitable mock object. np.isnan() can take non-arrrays as input. In my naive view it should do something like asarray()/asanyarray() internally, which seems the same as what np.array() is doing. So TBH, I don't really understand, how there can be objects o, that pass np.isnan(np.array(o)), but fail np.isnan(o). pytorch shows that they exist, but that's beyond my craft and I don't have the ambition to dig into this world for a test here. Feel free to push one if you understand the numpy machinery here. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
I could go either way on the backport. |
Sorry, something went wrong.
…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 applying np.nan() to an arbitrary object, but luckly, we have the converted array just one line later and can work with that.