Issue #8299, implemented copy, added test#8375
Conversation
lib/matplotlib/colors.py
Outdated
|
|
||
| def __copy__(self): | ||
| """Create new object with the same class and update attributes | ||
| If object is initialized, copy the elements of _lut list |
There was a problem hiding this comment.
Much like the test, I don't think you need to mention _lut in the doc here
lib/matplotlib/colors.py
Outdated
| If object is initialized, copy the elements of _lut list | ||
| """ | ||
| cls = self.__class__ | ||
| newCMapObj = cls.__new__(cls) |
There was a problem hiding this comment.
CamelCase is for classes, please use snake_case in the future.
There was a problem hiding this comment.
Should I change the case and remove the mention?
lib/matplotlib/tests/test_colors.py
Outdated
| def test_colormap_copy(): | ||
| cm = plt.cm.Reds | ||
| cm([-1, 0, .5, np.nan, np.inf]) | ||
| cm.set_bad('y') |
There was a problem hiding this comment.
@tacaswell Won't this leak into the global state of plt.cm.Reds? Why do we have global state anyway? Maybe the colormaps should be made read-only until copied?
There was a problem hiding this comment.
I think this will leak into the reds
There was a problem hiding this comment.
I am also not sure why the color maps are global like this. I assume it is an optimization from long ago?
There was a problem hiding this comment.
How should I change it?
There was a problem hiding this comment.
make a copy before using set_*
There was a problem hiding this comment.
Okay yes. That would prevent leaks into red
lib/matplotlib/tests/test_colors.py
Outdated
| def test_colormap_copy(): | ||
| cm = plt.cm.Reds | ||
| cm([-1, 0, .5, np.nan, np.inf]) | ||
| cm.set_bad('y') |
There was a problem hiding this comment.
I think this will leak into the reds
lib/matplotlib/tests/test_colors.py
Outdated
| expected1.set_over('c') | ||
| expected2.set_over('m') | ||
|
|
||
| assert_array_equal(cm._lut, expected1._lut) |
There was a problem hiding this comment.
I would rather test this by comparing the mapped values rather than reaching in at looking at the private attributes.
There was a problem hiding this comment.
How do I do that?
There was a problem hiding this comment.
Something like
ret1 = cm([-1, 0, .5, 1, np.nan, np.inf])
cm2 = copy.copy(cm)
cm2.set_bad(...)
ret2 = cm([-1, 5, .5, 1, np.nan, np.inf])
assert_array_equal(ret1, ret2)
There was a problem hiding this comment.
If I do the following :
cm = plt.cm.Reds
ret1 = cm([-1, 0, 0.5, 1, np.nan, np.inf])
cm2 = copy.copy(cm)
cm2.set_bad('g')
ret2 = cm([-1, 5, .5, 1, np.nan, np.inf])
assert_array_equal(ret1, ret2)I get the assertion error.
The copy function does work but how do I find the error?
There was a problem hiding this comment.
Because the input arrays are different (I apparently can not type).
|
Ping @vidursatija? |
|
I'll get back soon. I'm having exams. Just a week |
|
@vidursatija Hope you can get to this this weekend; it's really annoying in tests. |
|
Can you please navigate me? I'm getting a bit confused with the input arrays |
|
@vidursatija As far as I can tell, you just need to use the same values in |
QuLogic
modified the milestones:
2.0.1 (next bug fix release),
2.0.2 (next bug fix release)
|
By doing the following : cm = plt.cm.Reds
ret1 = cm([-1, 0, .5, 1, np.nan, np.inf])
cm2 = copy.copy(cm)
cm2.set_bad('g')
ret2 = cm([-1, 0, .5, 1, np.nan, np.inf])
assert_array_equal(ret1, ret2)I get a runtime warning but no assertion error. |
|
Sounds good to me. You might want to put those |
|
By doing the following : cm = plt.cm.Reds
with np.errstate(invalid='ignore'):
ret1 = cm([-1, 0, .5, 1, np.nan, np.inf])
cm2 = copy.copy(cm)
cm2.set_bad('g')
with np.errstate(invalid='ignore'):
ret2 = cm([-1, 0, .5, 1, np.nan, np.inf])
assert_array_equal(ret1, ret2)This works. Should I commit it? I'll do it ASAP |
|
Ping? |
tacaswell
modified the milestones:
2.1 (next point release),
2.0.3 (next bug fix release)
Implemented with no leak into Reds
phobson
changed the title
|
@tacaswell I think this is ready to go. Could you update your review? |
QuLogic
changed the title
4 participants
Closes #8299
Creating new PR to replace #8314 @phobson @tacaswell