Implemented __copy__ in Colormap #8314
Conversation
|
@phobson The code was kind of convoluted but the implementation wasn't that difficult |
lib/matplotlib/colors.py
Outdated
| newCMapObj = cls.__new__(cls) | ||
| newCMapObj.__dict__.update(self.__dict__) | ||
| if self._isinit: | ||
| newCMapObj._lut = np.copy(self._lut) |
There was a problem hiding this comment.
Please use spaces instead of tabs
|
Can you also add a test for this? |
lib/matplotlib/colors.py
Outdated
| """Create new object with the same class and update attributes | ||
| If object is initialized, copy the elements of _lut list | ||
| """ | ||
| cls = self.__class__ |
There was a problem hiding this comment.
Is this really the best way to do this? It seems to be dropping down to a low layer of python.
There was a problem hiding this comment.
The low layer of python is presumably faster than the normal python code. Another way of doing this would be creating a normal Colormap object and manually copying each attribute.
There was a problem hiding this comment.
How should I implement a test?
There was a problem hiding this comment.
Get a color map, use it (cm([-1, 0, .5, 1, np.nan, np.inf]), copy it, use set_bad, set_over, set_under on one or both of them, use them both and check that the colors come back as expected.
There was a problem hiding this comment.
I would not expect this to be much faster. If you think it is can you bench mark it?
There was a problem hiding this comment.
I will bench mark it and implement the tests. I'll post the results soon.
There was a problem hiding this comment.
I have bench marked the 2 functions.
The following code is used:
import matplotlib.pyplot as plt
import time
cm = plt.cm.Reds
start = time.time()
cm2 = cm.copy_1() #Using low level python
end1 = time.time()
cm3 = cm.copy_2() #Copying the attributes manually
end2 = time.time()
print("Method 1 :" + str(end1-start))
print("Method 2 :" + str(end2-end1))I get the following output:
Method 1 :1.8358230590820312e-05
Method 2 :2.6941299438476562e-05
I can definitely say that using low level python functions would lead to a simpler, faster code, which might be a little difficult to understand by seeing it.
Test
I've used the copy_1 function to copy the object.
import numpy as np
import matplotlib.pyplot as plt
cm = plt.cm.Reds
cm([-1, 0, .5, np.nan, np.inf])
cm.set_bad('y')
cm2 = cm.copy_1()
cm2.set_bad('b')
print(cm._rgba_bad)
print(cm2._rgba_bad)
cm.set_under('y')
cm2.set_under('r')
print(cm._rgba_under)
print(cm2._rgba_under)
cm.set_over('c')
cm2.set_over('m')
print(cm._rgba_over)
print(cm2._rgba_over)Output is as follows:
(0.75, 0.75, 0.0, 1)
(0.0, 0.0, 1.0, 1)
(0.75, 0.75, 0.0, 1)
(1.0, 0.0, 0.0, 1)
(0.0, 0.75, 0.75, 1)
(0.75, 0.0, 0.75, 1)
As we can see, using the function on both Colormaps doesn't change the other
There was a problem hiding this comment.
My last commit (3ece4cb) has used lower level python to implement copy. The code has also used spaces instead of tabs
|
Timing a single execution like that is not reliable, those are differences in the Before this change, that test would have passed (as the issue is that the |
|
The code used for benchmark is as follows : import matplotlib.pyplot as plt
import timeit
cm = plt.cm.Reds
print(timeit.timeit(cm.copy_1))
print(timeit.timeit(cm.copy_2))We get the following output : The lower level python copy function works faster. TestThe code used for test is as follows import numpy as np
import matplotlib.pyplot as plt
cm = plt.cm.Reds
cm([-1, 0, .5, np.nan, np.inf])
cm.set_bad('y')
cm2 = cm.copy_1()
cm2.set_bad('b')
cm.set_under('k')
cm2.set_under('r')
cm.set_over('c')
cm2.set_over('m')
print(np.array_equal(cm._lut, cm2._lut))We get the following output: The test is succesful. The array _lut is not shared and the values are copied. I'm sorry for the mistake in the previous test |
|
Can you put that test in the |
|
While putting the test, should I use the numpy testing utility? |
|
I've used the numpy testing utility to implement the test |
|
The Travis CI build is failing due to some other reason. It passes the copy test. There are no indentation problems. |
|
@phobson please can you review this PR |
There was a problem hiding this comment.
This looks good to me, but some it is a bit over my head. @tacaswell should have the final word.
|
@tacaswell please review it |
lib/matplotlib/tests/test_colors.py
Outdated
| cm2.set_under('r') | ||
| cm.set_over('c') | ||
| cm2.set_over('m') | ||
| assert_equal(np.array_equal(cm._lut, cm2._lut), False) |
There was a problem hiding this comment.
I am not a huge fan of this test, I would prefer
assert_array_equal(cm(data), expected1)
assert_array_equal(cm2(data), expected2)As that is the thing thing we actually care about (not the details of the internal state). If we change how the color mapping works internally, this test might not cover what we think it covers.
|
Can you please squash all of these commits into one commit with commit message that explains what is being fixed and why? |
|
I deleted the old repository. I forked and solved the issue. That is one commit. For that we'll have to close this pull request and create a new one. |
|
I'm having problems. I deleted the forked repository. I still have the changed files but I can't merge them here. Should I create a new pull request and merge all of it in a single commit? |
|
Closing in favor of #8375 |
I haven't tried executing. Only implemented copy in the Colormap class.
Issue #8299