Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize SpanningTree and graph-related methods #53

Closed
MischaPanch opened this issue Feb 24, 2021 · 2 comments
Closed

Generalize SpanningTree and graph-related methods #53

MischaPanch opened this issue Feb 24, 2021 · 2 comments
Labels
enhancement New feature or request

Comments

@MischaPanch
Copy link
Collaborator

MischaPanch commented Feb 24, 2021

Currently we support graph-related stuff only for geospatial data. If ever needed in a more general context (e.g. for arbitrary Euclidean or even non-Euclidean data, arbitrary triangulations a.s.o.), the classes there should be refactored and extended. Here a snippet of a conversation highlighting some of the problems with the current structure:


It is related to coordinates, since a Delaunay triangulation only makes sense for euclidean points. If we would like to have stuff only related to the graph representation, it shouldn't be tightly coupled to a special type of graph (representing a Delaunay triangulation). The class

class SpanningTree:
    """
    Wrapper around a tree-finding algorithm that will be applied on the Delaunay graph of the datapoints
    """
    def __init__(self, datapoints: np.ndarray, tree_finder: Callable[[nx.Graph], nx.Graph] = nx.minimum_spanning_tree):

should not depend on an np.ndarray, which has to be an array of euclidean coordinates, since a Delaunay triangualation is computed, but rather a nx.Graph. Moreover, paramter tree_finder allows to construct a subgraph, which may not be a spanning tree at all. Actually, the class only provides convenience methods for inspecting a weighted (nx) graph. So, either the class should be

class WeightedGraphWrapper:
    """
    Wrapper around a nx.Graph
    """
    def __init__(self, graph: nx.Graph):

Or, if you really would like to have a spanning tree, it may look like

class SpanningTree:
    def __init__(self, graph: nx.Graph, mode="min"):
        self.tree = nx.minimum_spanning_tree if mode== "min" else ...

(resp. including an enum for min max). In any case, I don't know if this is enough functionality, to justify a class.
By now, it is tightly coupled to euclidean point graphs (even more, Delaunay graphs), so I would put it into geoanalytics. What do you think?

Originally posted by @schroedk in #50 (comment)

@MischaPanch MischaPanch added enhancement New feature or request good first issue Good for newcomers labels Feb 24, 2021
@opcode81
Copy link
Owner

opcode81 commented Feb 24, 2021

The class SpanningTree doesn't do a whole lot and what it does is unlikely to be a useful abstraction in general. YAGNI, I daresay. I therefore propose that it be merged with its subclass. Generalising it would, accordingly, not be a good idea in my opinion. (Why was this class created in the first place?)
We could think about moving the Delaunay graph function outside of the geoanalytics package if it is ever needed in another context, but this really isn't a TODO item that should be tracked. I thus propose to close this issue.

@opcode81 opcode81 removed the good first issue Good for newcomers label Feb 24, 2021
@opcode81
Copy link
Owner

Closing this issue as there was no objection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants