pysal / momepy

Urban Morphology Measuring Toolkit
https://docs.momepy.org
BSD 3-Clause "New" or "Revised" License
496 stars 59 forks source link

API: distinction between libpysal and networkx graphs #579

Closed martinfleis closed 5 months ago

martinfleis commented 6 months ago

We currently use both libpysal and nx graphs, and unfortunately use the term graph for both. Compare this:

https://github.com/pysal/momepy/blob/889fb44d74f99c34acb491fd041f50b60d3a960b/momepy/graph.py#L67

and this

https://github.com/pysal/momepy/blob/889fb44d74f99c34acb491fd041f50b60d3a960b/momepy/functional/_distribution.py#L105

I don't think it is wise to do that. And since the contents of the graph.py module will likely not change signficantly, we should probably use better name within the new functional module for libpysal graphs to avoid confusion.

jGaboardi commented 6 months ago

Some initial ideas:

martinfleis commented 6 months ago

what about leaving "graph" and using spatial_weights?

jGaboardi commented 6 months ago

I was thinking something like that, but then thought it would be counterproductive in our general efforts to move away from the "weights" terminology on the PySAL side of things.

martinfleis commented 6 months ago

Are we trying to move away from weights terminology or just weights class?

u3ks commented 6 months ago

neighbors_graph ?

jGaboardi commented 6 months ago

Are we trying to move away from weights terminology or just weights class?

I guess I am not sure. I thought so, but maybe I misinterpreted. Perhaps we can get other devs' opinions on that?

neighbors_graph ?

As for that, I think it works well. We could even truncate it we wanted: neigh_graph.

martinfleis commented 6 months ago

Let's pick it up during the next PySAL dev meeting.

martinfleis commented 5 months ago

We have decided to keep the name Graph in libpysal. Here, we have functions that use one or the other. I will just add type hints to the graph module with explicit nx.Graph typing.