-
Notifications
You must be signed in to change notification settings - Fork 875
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
Have a dedicated neighborhood property and a get_neighborhood method on Cell #2309
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general design!
Any performance implication on caching? Mostly good probably, right?
Co-authored-by: Ewout ter Hoeven <[email protected]>
just a quick observation about cellspaces. A random walk is now a one liner: |
That’s really nice! I would appreciate a deprecationwarning for |
AFAIK that is not possible because the name clashes with the property. |
It looks like this does introduce some errors. |
for more information, see https://pre-commit.ci
Performance benchmarks:
|
These benchmark results are exactly opposite of #2308. So seems to cancel out and are just based on an outdated branch or whatever. |
Performance benchmarks:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was starting to type something about API compatibility and deprecation warnings, but then remembered this is all in the experimental space.
Love the experimental space!
Thanks, great to have it in! Please make sure that pre-commit passes before merging. Always a bitch to clean up (done in 7037a32). |
This picks up on a discussion in #2296. In the current code, we have method named
Cell.neighborhood
. This is an unfortunate name. So we agreed to split this into a property for the direct neighborhood (i.e. radius = 1) and aget_neighborhood
method for larger radii. We had a bit of discussion on the names, but I decided to keep it simple:neighborhood
andget_neighborhood
. I have updated the docstrings to clearly explain when to use which one.