New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow Paths to be marked as readonly #2010
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,13 +83,11 @@ def test_clip_to_bbox(): | |
ax.set_xlim([-18, 20]) | ||
ax.set_ylim([-150, 100]) | ||
|
||
star = mpath.Path.unit_regular_star(8) | ||
path = mpath.Path(star.vertices.copy(), star.codes) | ||
path = mpath.Path.unit_regular_star(8).deepcopy() | ||
path.vertices *= [10, 100] | ||
path.vertices -= [5, 25] | ||
|
||
circle = mpath.Path.unit_circle() | ||
path2 = mpath.Path(circle.vertices.copy(), circle.codes) | ||
path2 = mpath.Path.unit_circle().deepcopy() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is an example which does something similar too (http://matplotlib.org/examples/pylab_examples/marker_path.html). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I'll update the example as well. |
||
path2.vertices *= [10, 100] | ||
path2.vertices += [10, -25] | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
from matplotlib.path import Path | ||
from nose.tools import assert_raises | ||
|
||
def test_readonly_path(): | ||
def readonly(): | ||
path = Path.unit_circle() | ||
path.vertices = path.vertices * 2.0 | ||
|
||
assert_raises(AttributeError, readonly) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a little clunky. How about:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah -- I didn't know about the context manager support on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this property (and some of the others) really need to exist. The original attribute access was perfectly satisfactory - if you changed something that didn't make sense then you would expect it to blow up...
I guess what I'm saying is that I'm not really a big fan of protecting values in this way - if a user wants to fiddle with them, then they should be able to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nice thing about this approach is that when the vertices are updated, these values are updated as well, so they should always be correct. I think that's a good thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but there are always corner cases 😉
I'm happy with the read only protection for coding mistakes (such as mine) - but this is python: I don't think we need to worry about protecting the whole state of Path instances.
Either way, it's much of a muchness - and more important than whether it is a property or an actual attribute, I think there should be a oneline docstring for all of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll leave this as properties -- I think in most cases this is preferred.
Agreed about the docstrings.
I realise we can't protect much, but I think this was a particularly surprising one, mainly because I think users are surprised that
Path.unit_circle()
is a singleton. I could have also resolved this by having those return a copy, but that breaks some optimizations in markers and elsewhere where it tries to save the same path only once wherever possible.