Allow Paths to be marked as readonly #2010

Merged
merged 4 commits into from May 15, 2013

1 participant

@mdboom
Matplotlib Developers member

This PR is just to prevent the user from modifying the singleton Paths (unit_circle, unit_rectangle etc) which then cause nasty side effects for anything else that tries to use that Path later (such as the Marker infrastructure).

Issue #2003 recently "rediscovered" this bug, but we've seen it before.

1) Adds a readonly kwarg to the Path constructor
2) Mark all of the singleton paths as readonly
3) deepcopy those Paths in tests that need to modify the path.

The actual domain of this API change is fairly small, as most Paths created will still be mutable -- this only affects code that wants to modify one of the singleton Paths.

@mdboom mdboom Allow Paths to be marked as readonly, mark all of the singletons as r…
…eadonly, and deepcopy those Paths in tests that need to modify the path.
acbac82
@mdboom mdboom referenced this pull request May 15, 2013
Merged

Fixed hatch clipping. #2003

@pelson pelson commented on the diff May 15, 2013
lib/matplotlib/path.py
+ self._vertices = vertices
+ self._update_values()
+
+ @property
+ def codes(self):
+ return self._codes
+
+ @codes.setter
+ def codes(self, codes):
+ if self._readonly:
+ raise AttributeError("Can't set codes on a readonly Path")
+ self._codes = codes
+ self._update_values()
+
+ @property
+ def simplify_threshold(self):
@pelson
Matplotlib Developers member
pelson added a line comment May 15, 2013

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.

@mdboom
Matplotlib Developers member
mdboom added a line comment May 15, 2013

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.

@pelson
Matplotlib Developers member
pelson added a line comment May 15, 2013

I think that's a good thing.

I agree, but there are always corner cases 😉

path.vertices[10:50] += 10

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.

@mdboom
Matplotlib Developers member
mdboom added a line comment May 15, 2013

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson commented on the diff May 15, 2013
lib/matplotlib/tests/test_patches.py
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()
@pelson
Matplotlib Developers member
pelson added a line comment May 15, 2013

I think there is an example which does something similar too (http://matplotlib.org/examples/pylab_examples/marker_path.html).

@mdboom
Matplotlib Developers member
mdboom added a line comment May 15, 2013

Thanks. I'll update the example as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson and 1 other commented on an outdated diff May 15, 2013
lib/matplotlib/tests/test_path.py
@@ -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)
@pelson
Matplotlib Developers member
pelson added a line comment May 15, 2013

This seems a little clunky. How about:

c = Path.unit_circle()
with assert_raises(AttributeError):
    c.vertices *= 2
@mdboom
Matplotlib Developers member
mdboom added a line comment May 15, 2013

Ah -- I didn't know about the context manager support on assert_raises. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mdboom
Matplotlib Developers member

Ah -- it seems the with assert_raises context manager trick doesn't work on Python 2.6.

@mdboom mdboom merged commit 83d9cf8 into matplotlib:master May 15, 2013

1 check was pending

Details default The Travis CI build is in progress
@mdboom mdboom deleted the mdboom:immutable-paths branch Aug 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment