added fig and sub as parameters for plot creation #500

ahoc · 2018-11-07T08:54:46Z

Use like:
cp = nv.CircosPlot(G, fig=30, sub=(1,2,1))
to avoid automatic creation of new figure.

Closes #499.

Use like: cp = nv.CircosPlot(G, fig=30, sub=(1,2,1)) to avoid automatic creation of new figure.

Use like: import nxviz as nv import palettable.colorbrewer as pc cp = nv.CircosPlot(G, cmap=pc.sequential.Reds_9) cp.draw()

ericmjl

@ahoc thanks for the PR! Your proposed code introduces some bigger changes than I had originally thought about. Before I'm ready to accept this PR, I'd like to ensure that those changes ensure that the nxviz declarative API is preserved, that conditional logic is minimized, or if necessary, then tested against the appropriate combinations of conditionals.

Could you, then, do the following?

Add some tests into the test suite for catching cmap conditions for node colors?
Provide your thoughts on the three API options I have outlined?
Add yourself to the list of contributors in AUTHORS.rst?

In case you're thinking of implementing more features, I'd ask you to hold off, or at least do it in a separate separate branch (i.e. ahoc:feature-branch) and put in a separate PR. Doing so makes it much easier to review this PR.

ericmjl · 2018-11-07T15:29:10Z

nxviz/plots.py

@@ -153,7 +153,12 @@ def __init__(
        figsize = (6, 6)
        if "figsize" in kwargs.keys():
            figsize = kwargs["figsize"]
-        self.figure = plt.figure(figsize=figsize)
+        if "fig" in kwargs.keys():


The API you've proposed here uses **kwargs. I'm cool with the idea, but I also want to be thoughtful about the API. nxviz's API is its biggest 'selling point', so I put more oversight on the API than I do on features. Code maintainability is important too, though it only takes a slight backseat to the API.

I was wondering if we could do an API design thought experiment here.

For the use case where we are plotting a CircosPlot to an existing figure, which one looks better to you?

Option 1, which uses idioms from the PyData ecosystem

# Pre-instantiate a figure and axes object. fig = plt.figure() ax = fig.add_subplot(111) c = CircosPlot(G, ..., ax=ax)

Option 2, your current proposed API

c = CircosPlot(G, ..., fig=30, sub=(1,1,1)

Option 3, a "fig_kwargs" API

fig_kwargs = dict( figsize=(10, 10), subplots=(1,1,1) ) c = CircosPlot(G, ..., fig_kwargs=fig_kwargs)

Of the three, I personally lean most towards option 1, which plays nicely with how people use other Python packages. Of the remaining two, I have no strong opinions. If I were to weight the three options, out of 100 points, I would assign 70-15-15.

What are your thoughts?

I agree, 1 is certainly better than 2 (but requires more changes?). 3 could be an option if one would plan to later implement more options and have them nicely bundled. So: 70-10-20.

@ahoc I think option 1 only requires an isolated conditional: if ax is not specified, then make a figure and an axes object. In this case, ax should be None by default.

ericmjl · 2018-11-07T15:31:27Z

nxviz/plots.py

-            self.figure.colorbar(self.sm, cax=cax)
+            #self.figure.subplots_adjust(right=0.8)
+            #cax = self.figure.add_axes([0.85, 0.2, 0.05, 0.6])
+            #self.figure.colorbar(self.sm, cax=cax)


After uncommenting the above three lines, have you run tests locally on your computer to check that the tests pass?

The three commented lines are the original ones (Sorry, I commented instead of deleting them). If, as I suggest they are replaced by:

Suggested change

#self.figure.colorbar(self.sm, cax=cax)

self.figure.colorbar(self.sm, ax=self.ax)

thus plotting to ax and not to a newly created cax it works also for several subplots in one figure. But the colorbars look a bit narrower. I will add some examples to the test suite.

Got it! Yes, please add a few tests.

Btw, I would avoid putting actual plotting code in the test suite. I have not yet set up plotting tests properly. You can test for logical properties of the objects instead.

ericmjl · 2018-11-07T15:33:22Z

nxviz/plots.py

        self.ax.relim()
        self.ax.autoscale_view()
        self.ax.set_aspect("equal")

-    def compute_node_colors(self):
+    def compute_node_colors(self, **kwargs):


The **kwargs pattern I have seen thus far leaves options tough to document.

What are your thoughts on the following suggested change?

Suggested change

def compute_node_colors(self, **kwargs):

def compute_node_colors(self, cmap: str=None):

"""

:param cmap: The matplotlib colormap to use.

"""

It would then precipitate the following change in line 259 below.

Much better, that was just a hack.

Ok, please accept the changes; you can do so here, or just copy/paste it into your own branch.

ericmjl · 2018-11-07T15:35:32Z

nxviz/plots.py

@@ -250,6 +256,9 @@ def compute_node_colors(self):
        elif dtype == "continuous" and is_data_diverging(data):
            cmap = get_cmap(cmaps["diverging"].mpl_colormap)

+        if "cmap" in kwargs.keys():
+            cmap = get_cmap(kwargs["cmap"].mpl_colormap)


Lines 259 and 260 optionally change cmap if it is set. This may override the above conditions. What are your thoughts here?

Will manually selecting a cmap result in unintended behavior that are not currently caught by the tests?

I don't think so, but can do some more tests. It would be better though to put it before the above conditons.

@ahoc thanks for getting back.

Yes, please add a few tests for the different options that might show up. Rather than think of the generalized cases, having a few specific cases first is sufficient. (I don't want to over-complicate the testing up-front, we can discover the general cases later.)

ericmjl · 2018-12-05T00:36:25Z

@ahoc, please let me know if you're still interested in completing this PR.

ahoc added 3 commits November 7, 2018 09:52

added fig and sub as parameters for plot creation

d4bcc1d

Use like: cp = nv.CircosPlot(G, fig=30, sub=(1,2,1)) to avoid automatic creation of new figure.

Update plots.py

a0c43b2

added colormap option

01734df

Use like: import nxviz as nv import palettable.colorbrewer as pc cp = nv.CircosPlot(G, cmap=pc.sequential.Reds_9) cp.draw()

ericmjl requested changes Nov 7, 2018

View reviewed changes

This was referenced Nov 7, 2018

Update AUTHORS.rst #501

Closed

plotting into figure and axis #499

Open

ahoc and others added 2 commits November 8, 2018 16:35

Update AUTHORS.rst

1a10300

Merge branch 'master' into master

94bd7de

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added fig and sub as parameters for plot creation #500

added fig and sub as parameters for plot creation #500

ahoc commented Nov 7, 2018 •

edited by ericmjl

Loading

ericmjl left a comment

ericmjl Nov 7, 2018

ericmjl Nov 7, 2018

ahoc Nov 7, 2018

ericmjl Nov 7, 2018

ahoc Nov 8, 2018

ericmjl Nov 7, 2018

ahoc Nov 7, 2018

ericmjl Nov 7, 2018

ericmjl Nov 7, 2018

ahoc Nov 7, 2018

ericmjl Nov 7, 2018

ericmjl Nov 7, 2018

ahoc Nov 7, 2018

ericmjl Nov 7, 2018

ericmjl commented Dec 5, 2018

	#self.figure.colorbar(self.sm, cax=cax)
	self.figure.colorbar(self.sm, ax=self.ax)

-    def compute_node_colors(self, **kwargs):
+    def compute_node_colors(self, cmap: str=None):
+        """
+        :param cmap: The matplotlib colormap to use.
+        """

added fig and sub as parameters for plot creation #500

Are you sure you want to change the base?

added fig and sub as parameters for plot creation #500

Conversation

ahoc commented Nov 7, 2018 • edited by ericmjl Loading

ericmjl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmjl commented Dec 5, 2018

ahoc commented Nov 7, 2018 •

edited by ericmjl

Loading