Default attributes for data structures #873
Conversation
| self.edge = {} | ||
| self.adjacency = {} | ||
| self.default_node_attributes = {} | ||
| self.default_node_attributes = {'x': 0.0, 'y': 0.0, 'z': 0.0} |
There was a problem hiding this comment.
I'm surprised to see this. I thought Graphs were purely topological.
There was a problem hiding this comment.
they are. before this was added in the network init. however, even graphs have to be visualised so purely from a practical point of view we might as well add the default coordinates here. can change this back if it doesn't feel right ...
There was a problem hiding this comment.
in general, i think the separation between graph and network, and between halfedge and mesh, etc. is a bit artificial and is only there to make the individual class definitions a bit shorter. especially in case of the mesh it doesn't make much sense because halfedge is not meaningful by itself...
There was a problem hiding this comment.
If a graph were visualized with all the nodes at the origin, it's not much of a visualization...
There was a problem hiding this comment.
indeed it is not. the defaults are just there such that more meaningful coordinates can be set through the attribute functions. if no geometry is explicitly assigned, the graph could be considered as purely topological and visualisations are meaningless without a chosen layout. we could wrap the layout mechanisms of networkx for this...
There was a problem hiding this comment.
I would vote for not having coords in the graph and keeping it as purely topological as possible. Then, as mentioned, visualization needs layouting, but we can either use some of the networkx ones, or our own, I wrote a few simple ones for our workshops, which surely need improvement but might be a starting point.
There was a problem hiding this comment.
sure will move it to network
gonzalocasas
left a comment
gonzalocasas
left a comment
There was a problem hiding this comment.
Besides the comment about moving coordinates out of Graph, it lgtm. If possible, throw in a couple of unit tests to verify basic default attr behavior.
|
closed in favoour of #892 |
What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.