FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Issue #8299, implemented copy, added test by vidursatija · Pull Request #8375 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Issue #8299, implemented copy, added test - #8375

Merged
tacaswell merged 3 commits into
matplotlib:masterfrom
vidursatija:i_8299
May 25, 2017
ConversationCommits (3)ChecksFiles changed
Merged

Issue #8299, implemented copy, added test#8375
tacaswell merged 3 commits into
matplotlib:masterfrom
vidursatija:i_8299

Conversation

vidursatija commented Mar 24, 2017 •
edited by phobson
Loading

Copy link
Copy Markdown
Contributor

Closes #8299

Creating new PR to replace #8314 @phobson @tacaswell

NelleV mentioned this pull request Mar 24, 2017
Closed
QuLogic added this to the 2.0.1 (next bug fix release) milestone Mar 24, 2017
QuLogic requested review from phobson and tacaswell March 24, 2017 23:01
QuLogic reviewed Mar 24, 2017
View reviewed changes
Comment thread 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

QuLogic Mar 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Much like the test, I don't think you need to mention _lut in the doc here

Comment thread lib/matplotlib/colors.py Outdated
If object is initialized, copy the elements of _lut list
"""
cls = self.__class__
newCMapObj = cls.__new__(cls)

QuLogic Mar 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

CamelCase is for classes, please use snake_case in the future.

vidursatija Mar 25, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should I change the case and remove the mention?

QuLogic Mar 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Go ahead.

QuLogic reviewed Mar 26, 2017
View reviewed changes
Comment thread 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')

QuLogic Mar 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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?

tacaswell Apr 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this will leak into the reds

tacaswell May 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am also not sure why the color maps are global like this. I assume it is an optimization from long ago?

vidursatija May 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How should I change it?

tacaswell May 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

make a copy before using set_*

vidursatija May 14, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Okay yes. That would prevent leaks into red

tacaswell previously requested changes Apr 3, 2017
View reviewed changes

tacaswell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Improvements to tests.

Comment thread 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')

tacaswell Apr 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this will leak into the reds

Comment thread lib/matplotlib/tests/test_colors.py Outdated
expected1.set_over('c')
expected2.set_over('m')

assert_array_equal(cm._lut, expected1._lut)

tacaswell Apr 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would rather test this by comparing the mapped values rather than reaching in at looking at the private attributes.

vidursatija Apr 4, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How do I do that?

tacaswell Apr 4, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

vidursatija Apr 6, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

tacaswell Apr 9, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Because the input arrays are different (I apparently can not type).

QuLogic commented Apr 21, 2017

Copy link
Copy Markdown
Member

Ping @vidursatija?

vidursatija commented Apr 22, 2017

Copy link
Copy Markdown
Contributor Author

I'll get back soon. I'm having exams. Just a week

QuLogic commented Apr 29, 2017

Copy link
Copy Markdown
Member

@vidursatija Hope you can get to this this weekend; it's really annoying in tests.

vidursatija commented Apr 30, 2017

Copy link
Copy Markdown
Contributor Author

Can you please navigate me? I'm getting a bit confused with the input arrays

phobson commented Apr 30, 2017

Copy link
Copy Markdown
Member

@vidursatija As far as I can tell, you just need to use the same values in ret1 and ret2. Tom had a typo in his previous message.

QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0.2 (next bug fix release) May 3, 2017

vidursatija commented May 4, 2017

Copy link
Copy Markdown
Contributor Author

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.
Should I commit this change?

QuLogic commented May 4, 2017

Copy link
Copy Markdown
Member

Sounds good to me. You might want to put those cm() in with np.errstate(invalid='ignore'): since those nan/inf are intentional.

vidursatija commented May 4, 2017

Copy link
Copy Markdown
Contributor Author

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

QuLogic commented May 11, 2017

Copy link
Copy Markdown
Member

Ping?

tacaswell modified the milestones: 2.1 (next point release), 2.0.3 (next bug fix release) May 12, 2017
Issue matplotlib#8375 and matplotlib#8299 fixed
1579364 
Implemented with no leak into Reds
phobson changed the title Issue #8299, implemented copy, added test [MRG+1] Issue #8299, implemented copy, added test May 18, 2017

phobson commented May 18, 2017

Copy link
Copy Markdown
Member

@tacaswell I think this is ready to go. Could you update your review?

QuLogic mentioned this pull request May 21, 2017
Merged
tacaswell dismissed their stale review May 25, 2017 02:48

Issues addressed

tacaswell merged commit d50843e into matplotlib:master May 25, 2017
QuLogic changed the title [MRG+1] Issue #8299, implemented copy, added test Issue #8299, implemented copy, added test Jun 8, 2017
richeshgupta mentioned this pull request Sep 23, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tacaswell tacaswell left review comments
QuLogic QuLogic left review comments
phobson phobson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.1

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL