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

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

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

martinfleis
Copy link
Member

Closes #675

@ljwolf
Copy link
Member

ljwolf commented Jan 15, 2024

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

Actually not because y does not have the MultiIndex assigned.

@martinfleis
Copy link
Member Author

@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
Copy link
Member

ljwolf commented Jan 17, 2024

Fine with that!

@martinfleis
Copy link
Member Author

Added aggregate and allowed callable as transformation in transform.

@martinfleis martinfleis changed the title ENH: add Graph.apply ENH: add Graph.apply, Graph.aggregate and allow callable as transformation in transform Jan 17, 2024
@martinfleis
Copy link
Member Author

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

Copy link
Member

@ljwolf ljwolf left a comment

Choose a reason for hiding this comment

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

looks good to me!

Copy link
Member

@knaaptime knaaptime left a comment

Choose a reason for hiding this comment

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

sweet

@ljwolf ljwolf merged commit e70cae1 into pysal:main Jan 17, 2024
9 checks passed
@martinfleis martinfleis deleted the apply branch January 17, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interest in Graph.apply?
4 participants