Skip to content

Conversation

@iosonofabio
Copy link
Member

@iosonofabio iosonofabio commented Apr 4, 2023

Back in September 2022 @tacaswell suggested looking into container artists as a way of tidying up our matplotlib code, e.g. in grave:

https://github.com/networkx/grave

While that repo is barely alive, it does have a proof of concept that could be used here.

The goal would be to have access to an object that represents the entire visual graph, something like:

g = ig.Graph.Full(3)
fig, ax = plt.subplots()
visual_graph = ig.plot(g)

# Now we could manipulate the object
visual_graph.set_vertex_color("green")
visual_graph.set_edge_width(2)

It might take a while to get this in place, but I thought starting a draft does not hurt. Feedback welcome: on the idea for now, ignore the details of the code.

TODO:

  • check text offset and angle
  • tidy up code (design AND implementation)
  • see if it's appropriate to fix a few of those TODOs in the code
  • take away FIXME about PathCollections, we won't implement those for now, too much work
  • verify remaining images (probably fix convex hull)
  • verify strange behaviour with edit test (something about interactive mode I guess)
  • actually fix contains and pickup callbacks
  • serially add methods as above, e.g. set(vertex_color=...) would apply to all vertices, set_vertex_color(...) would also apply to all vertices.
  • fix the set method, rotation bug! 😂

@iosonofabio iosonofabio changed the title Matplotlb Container Artist Matplotlib Container Artist Apr 4, 2023
@iosonofabio iosonofabio changed the base branch from develop to main April 4, 2023 04:41
@iosonofabio
Copy link
Member Author

@tacaswell I hereby summon you from the depths of the web! Please help us 🙇

  • I have refactored our matplotlib code to encapsulate all of our artists into a container artist called GraphArtist (name could be better yes).
  • The code runs without errors, but nothing is shown, i.e. the container artist and its children are not drawn onto the axes.
  • I tried to follow the breadcrumbs in grave, but I don't see any actual call to Artist.draw there, only Artist._reprocess:

https://github.com/networkx/grave/blob/944049f4fa639811b477e50e07ade5b5d1a0a7bb/grave/grave.py#L397

My understanding is that you instantiate the primitive children with some knowledge about their axes (e.g. via transform=ax.transData) and then you also inject the figure and axes properties directly into the children as well. Moreover, the draw method of the container artist requests all children to be drawn as well, I get that. But when if the container drawn at all?

Thank you for your wisdom

@iosonofabio
Copy link
Member Author

Actually, this is starting to work. We already inject the Axes into vertex/edge builders as context, which is further injected into shapes.py as ctx (probably we should split that file by backend...). All that was left to do was to use the injected ctx to set ctx.transData as transformer for the patches. It seems to work partially now.

Onwards to vertex and edge labels, those are Text instances so a little different including angles, but grave's code is not that difficult so I'm hoping this will converge soon.

@iosonofabio
Copy link
Member Author

Alright, this now produces plots that make sense. Tests don't pass because of padding in the axes limits I believe. I'll track this down over the next few days.

@iosonofabio
Copy link
Member Author

It's mostly the padding with the tests, but there are also a few legit failures with convex hull and post-draw editing. I think something is fishy about stale states for children.

@szhorvat
Copy link
Member

szhorvat commented Apr 5, 2023

you sure the update of clustering_directed.png is fine?

@iosonofabio
Copy link
Member Author

hehe good catch, I haven't finished refactoring the hull yet

@tacaswell
Copy link

I will try to look at this tomorrow.

@iosonofabio
Copy link
Member Author

Thanks @tacaswell

Just to give you an overview of the current situation, it all works well except for stale bubbling from primitives. For instance:

graph_artist = GraphArtist(...)
vertex_artist = graph_artist.get_vertices()[0]
vertex_artist.set_facecolor("blue") 
# this sets the vertex_artist.stale -> True, which then sets graph_artist.stale -> True

Even though the bubbling of the stale propagates to graph_artist, that ends up triggering _reprocess which nukes the state but does not remove any artist from the actual canvas, creating new artists instead. So I think the logic with stale inside of GraphArtist.draw is wrong.

@iosonofabio iosonofabio marked this pull request as ready for review April 6, 2023 05:32
@tacaswell
Copy link

That looks correct to me. The way rendering works in Matplotlib (leaving aside blitting) is always a complete re-render. That is in the case of the raster backends we start with an empty RGBA buffer and then composite everything in.

The stale mechanism is a way of tracking "the Matplotlib view of this object has changed from the last time it drew, please re-draw" so you should not gate anything in draw on if it was stale or not. The draw might be triggered by something as benign as "please draw with a different backend" or "please draw with a different dpi".

To that end you should also avoid marking things as stale during the draw call. There is a bunch of logic to try and short-circuit that process, but if you are seeing drawing the figure triggering additional draws that is a bug.

There is also draw_idle which means "dear GUI framework, the next time you get a chance please re-draw and re-paint the image of the figure" so that we can effectively batch many changes without paying for a full render each time.

@iosonofabio
Copy link
Member Author

iosonofabio commented Apr 6, 2023

Thank you @tacaswell ! Any suggestions about the implementation of stuff like GraphArtist.set(vertex_facecolor="green")? Otherwise I'll just give it a go.

@iosonofabio iosonofabio marked this pull request as draft April 6, 2023 22:48
@iosonofabio
Copy link
Member Author

iosonofabio commented Apr 7, 2023

I implemented the set method that enables post-plot changes with the same keyword arguments as the initial call, e.g.:

import matplotlib.pyplot as plt
import igraph as ig
g = ig.Graph.Ring(5)
ig.plot(g, vertex_label=['a', 'b', 'c', 'd', 'e'], edge_label=['e1', 'e2', 'e3'], vertex_label_dist=1.1, vertex_label_angle=3.14/2)
plt.ion()
plt.show()

makes:

Figure1

Then we can call:

graphartist = plt.gca().get_children()[0]
graphartist.set(vertex_label_dist=1.5)

and we get:

Figure2

We might want to document this feature somehow.

In terms of drawing times, it does draw twice the first time, but then only once for each set call. I'm not entirely sure why.

edit: the figure rotation thing was a misclick from my end, not a bug 😸

@tacaswell
Copy link

If you are using constrained layout it will draw twice (once to sort out how big everything is once to actually draw).

I would not document digging into the axes artist list, it is better if ig.plot() returns the object the users need to interact with (being lazy and not looking up if you already do that and just did not in the example code).

I am also confused why the ring rotated and changed colors when you adjusted the label spacing....

@iosonofabio
Copy link
Member Author

Thanks @tacaswell .

  • Alright, redraw is as I thought, good.
  • return is a little tricky, it now returns the Axes and that'd be API breaking, but I agree with you. We might get an API change in the next major release.
  • rotation, omg that's funny. Let me look into it.

@iosonofabio
Copy link
Member Author

Okay, I now checked that the rotation was not a bug, just me being silly. I also added a decorator to add a bunch of setter methods all at once, so you can artist.set(vertex_color=...) or artist.set_vertex_color(...) and obtain the same result.

I also back propagated the container artist up the caller stack until igraph.plot. I realised that for Cairo, we return the drawer class which is the one that enables easy manipulation, but for matplotlib and plotly we return some kind of canvas (the Axes in mpl's case). That is inconsistent and I think we should return the Graph artist for matplotlib, as recommended by @tacaswell . However that's an API break, so we may stave it off until the next release... or not?

@natmas what do you think?

Based on that I'll modify the user docs a little to include a tutorial where the new container artist is showcased and manipulated, to let the users know about their newly acquired powers.

@iosonofabio iosonofabio marked this pull request as ready for review April 8, 2023 04:47
@iosonofabio
Copy link
Member Author

Hello from Okinawa!

Shall we merge this?

"set_picker",
)
)
class Graph(Artist, AbstractGraphDrawer):
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this class to GraphArtist instead to avoid confusion with the Graph class (especially when this class starts to appear in the index of the API documentation, right next to the "real" Graph class).

Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to rename the reference to this class from the tutorial if we decide to do the renaming.

@iosonofabio
Copy link
Member Author

I can rename, I'll be back next week and will do it

@ntamas
Copy link
Member

ntamas commented Apr 21, 2023

I can do it myself and merge if you have no objections against it.

@iosonofabio
Copy link
Member Author

iosonofabio commented Apr 21, 2023 via email

@ntamas ntamas merged commit 12d2195 into igraph:main Apr 24, 2023
@ntamas
Copy link
Member

ntamas commented Apr 24, 2023

This seems to have broken the Readthedocs doc build, see here: https://readthedocs.org/projects/igraph/builds/20263052/

Any idea what's going on here @iosonofabio ?

@iosonofabio
Copy link
Member Author

It looks like plotting an empty graph... Can fix within the week

@szhorvat
Copy link
Member

szhorvat commented Jun 7, 2023

@iosonofabio Could this be related to this PR, any hints? --> #677

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants