pysal / libpysal

Core components of Python Spatial Analysis Library
http://pysal.org/libpysal
Other
259 stars 78 forks source link

ENH: add Graph.apply, Graph.aggregate and allow callable as transformation in transform #676

Closed martinfleis closed 8 months ago

martinfleis commented 8 months ago

Closes #675

ljwolf commented 8 months ago

great implementation! two small thoughts

  1. I think that it might be good to support applying func over the weights. So, something like graph.apply('median') gives you the median weight in each neighbor set. I don't mind if this looks like graph.apply(None, 'median', **kwargs)...
  2. ...but this probably instead should look like graph.apply('median', values=None, **kwargs), which matches the func-first way that grouper.apply() and functools.reduce() is written?
martinfleis commented 8 months ago

So apply func over weights if values are not set and otherwise ignore weights and apply func over values? I am on the verge whether it can be potentially confusing or not, meaning if it should not be two methods rather than one.

jGaboardi commented 8 months ago

I am on the verge whether it can be potentially confusing or not, meaning if it should not be two methods rather than one.

I agree on the "potentially confusing" part and think 2 distinct methods might be a good idea.

knaaptime commented 8 months ago

nice. two quick reactions

  1. i think the really nice thing about the graph, especially compared to the old implementation, is how easy it is to do these kind of manipulations manually because of the way the pandas structure works underneath. An apply func is nice, but i think it would actually be more useful to demo these kinds of things in a tutorial so folks know how to actually manipulate a graph at will. You couldnt do that kinda thing with a W

  2. couldnt this just be g._adjacency.groupby(level=0).transform(func)? like i initially sketched for the lags?

martinfleis commented 8 months ago

I believe that both things are useful. Manual manipulation as well as both options of apply discussed above. I'll be reusing this code often and it is better to have a tested implementation I can just use than writing groupby from scratch every time.

You're right about the implementation though the outcome will be the same :). But I'll change that.

martinfleis commented 8 months ago

Actually not because y does not have the MultiIndex assigned.

martinfleis commented 8 months ago

@ljwolf what about including this alongside apply to cover your use case of applying a func over weights?

def aggregate(self, func):
    return self._adjacency.groupby(level=0).agg(func)

And we should probably also allow callable as transformation within transform, passed to self._adjacency.groupby(level=0).transform(func)

ljwolf commented 8 months ago

Fine with that!

martinfleis commented 8 months ago

Added aggregate and allowed callable as transformation in transform.

martinfleis commented 8 months ago

It will need some examples but that applies to the rest of Graph as well :).