Git issue #7216 - Add a "ruler" tool to the plot UI by terranjp · Pull Request #8774 · matplotlib/matplotlib

terranjp · 2017-06-18T15:19:58Z

PR Summary

A ruler as described in issue #7216. It is heavily inspired by the ruler in ImageJ. This is my first PR to an open source project, and my first code that will be reviewed by others. I tried my best to adhere to the developer guide and the style the other widgets are written in. Feedback and constructive criticism would be much appreciated.

Issue #7216

PR Checklist

Has Pytest style unit tests
I could not find tests for the other widgets and I am not sure how to go about testing it. Help would be appreciated if this addition requires tests.
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/whats_new.rst if major new feature
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

6

tacaswell · 2017-06-18T18:33:17Z

+
+    For the ruler to remain responsive you must keep a reference to it.
+
+     Parameters


When using numpydoc, you do not need to add markup to the parameter names (it is automatic) and can only use a limited number of heading names (see https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt)

tacaswell · 2017-06-18T18:35:46Z

+        x0 = self.ax.get_xlim()[0]
+        y0 = self.ax.get_ylim()[0]
+
+        self.ruler, = self.ax.plot([x0, x0], [y0, y0], label='ruler',


Might not want to give it a label, this will make it appear in legends by default.

I removed it. This is a leftover from when I attempted to use a legend to display the measure.

tacaswell · 2017-06-18T18:37:48Z

+        # default marker props. Don't really want to override the entire props
+        # if a user only gives one value.
+
+        if markerprops is not None:


default_markerprops does not get re-used else where so you can do this is

used_markerprops = dict(...) if markerprops is not None: used_markerprops.update(markerprops)

Nice. This cleans it up a bit.

tacaswell · 2017-06-18T18:38:11Z

+        else:
+            used_markerprops = default_markerprops
+
+        if textprops is not None:


Same comment as with markerprops above.

Same as markerprops above

tacaswell · 2017-06-18T18:39:18Z

+        self.axes_text = self.ax.annotate(s='', **used_textprops)
+        self.ax.add_artist(self.axes_text)
+
+        self.marker_a, = self.ax.plot((x0, y0), **used_markerprops)


These could all be one line as they all have the same marker, just use linestyle 'none'.

tacaswell · 2017-06-18T19:08:54Z

+        string will be appended to the length text annotations. 
+
+        """
+        AxesWidget.__init__(self, ax)


How many of the inputs can be passed to the base-class init instead of dealing with them explicitly here?

tacaswell · 2017-06-18T19:08:55Z

+                                 bbox=bbox)
+
+        x0 = self.ax.get_xlim()[0]
+        y0 = self.ax.get_ylim()[0]


I suggest initializing to np.nan.

tacaswell · 2017-06-18T19:08:57Z

+        string will be appended to the length text annotations. 
+
+        """
+        AxesWidget.__init__(self, ax)


How many of the inputs can be passed to the base-class init instead of dealing with them explicitly here?

tacaswell · 2017-06-18T19:10:47Z

+                        self.marker_b,
+                        self.marker_c]
+
+    def connect_events(self):


Should probably also listen for draw events to help with managing the blitting.

tacaswell · 2017-06-18T19:10:47Z

+                        self.marker_b,
+                        self.marker_c]
+
+    def connect_events(self):


Should probably also listen for draw events to help with managing the blitting.

tacaswell · 2017-06-19T03:30:03Z

I left a bunch of comments, but over all 👍 Please take many of them as questions, if there is a reason you did something that I commented on, explain why you did it that way!

This is not the simplest topic to take on as a first PR as anything involving blitting + user input has a large number of corner cases!

terranjp · 2017-06-19T13:50:31Z

Thanks for the feedback! When I get a chance I will implement/respond to your comments. I kinda realized awhile ago that it might be a bit much for my first PR. But if nothing else its a great challenge and learning experience!

…

On Sun, Jun 18, 2017 at 10:30 PM, Thomas A Caswell ***@***.*** > wrote: I left a bunch of comments, but over all 👍 Please take many of them as questions, if there is a reason you did something that I commented on, explain why you did it that way! This is not the simplest topic to take on as a first PR as anything involving blitting + user input has a large number of corner cases! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8774 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APvd_3zccXLsP5afb9eoCp5rVTF_YfU5ks5sFetGgaJpZM4N9heV> .

anntzer · 2017-06-20T03:10:34Z

I think a lot of attributes on the class could be made private. (Anything that's not private is going to become a backcompatibility nightmare at one point or another.)

Also this needs some testing, see test_widgets.py.

terranjp · 2017-06-20T11:41:03Z

@anntzer Agreed. I will change as many as attributes as possible to private. Also thanks for the tip on test_widgets.py. I will write tests for it.

due to it being very old

jklymak · 2018-05-30T21:23:11Z

@terranjp Any intetrest in still pushing this through?

This looks well-written, and some care has gone into it. OTOH, I can see the use, but it seems very fiddly, and one ruler implimentation will suit some needs, but not be useful for other needs. I almost wonder if this should be made a tutorial example or a self-contained library? Edge-case tools like this may not belong in the core library?

anntzer · 2018-05-31T02:52:19Z

I think this should be a tutorial/third-party lib.

terranjp · 2018-05-31T05:48:43Z

@anntzer @jklymak I agree. I think it should be a third party library. It would be easier to make it a little less fiddly and easier for people to hack on it. I've been wanting to get back to it as I have some ideas I would implement and I would like to package it up correctly. A tutorial would also be helpful as it is not immediately apparent how to create something like this in matplotlib. I will try and devote some time to it.

jklymak · 2018-05-31T16:16:02Z

@terranjp OK, I'll close.

If you end up with something that is rock solid that you think really has general benefit let us know.

Note that there will be a toolbar manager at some point with extensible tools so a user could easily plug in a widget like this, rather than having it be something everyone has. Keep an eye out for it - I'm not sure how we will advertise non-native plugins, but ideally there will be a list of related projects somewhere.

Thanks a lot for the contribution, and I hope you will consider adding other functionality!

Git issue #7216 - A simple ruler widget

fe49f30

tacaswell added this to the 2.1 (next point release) milestone Jun 18, 2017

tacaswell previously requested changes Jun 18, 2017

View reviewed changes

Added tests

aa4a529

tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017

dstansby added the status: needs review label Nov 22, 2017

afvincent modified the milestones: needs sorting, v3.0 Feb 15, 2018

jklymak closed this May 31, 2018

QuLogic removed the status: needs review label Jul 5, 2018

anntzer mentioned this pull request Dec 7, 2018

Add a "ruler" tool to the plot UI #7216

Closed


		For the ruler to remain responsive you must keep a reference to it.

		Parameters

Conversation

terranjp commented Jun 18, 2017

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jun 19, 2017

Uh oh!

terranjp commented Jun 19, 2017 via email

Uh oh!

anntzer commented Jun 20, 2017

Uh oh!

terranjp commented Jun 20, 2017

Uh oh!

jklymak commented May 30, 2018

Uh oh!

anntzer commented May 31, 2018

Uh oh!

terranjp commented May 31, 2018

Uh oh!

jklymak commented May 31, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants