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

Deprecate neighbor_iter in favor of iter_neighbors #1184

Merged
merged 1 commit into from
Mar 23, 2022

Conversation

rht
Copy link
Contributor

@rht rht commented Mar 8, 2022

See #980.

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #1184 (b1fab1d) into main (68b6631) will decrease coverage by 0.29%.
The diff coverage is 0.00%.

❗ Current head b1fab1d differs from pull request most recent head bcb71ed. Consider uploading reports for the commit bcb71ed to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1184      +/-   ##
==========================================
- Coverage   89.38%   89.09%   -0.30%     
==========================================
  Files          19       19              
  Lines        1300     1302       +2     
  Branches      264      264              
==========================================
- Hits         1162     1160       -2     
- Misses        100      104       +4     
  Partials       38       38              
Impacted Files Coverage Δ
mesa/space.py 93.88% <0.00%> (-1.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68b6631...bcb71ed. Read the comment docs.

warn(
"`neighbor_iter` is deprecated in favor of `iter_neighbors` "
"and will be removed in the subsequent version."
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice I intentionally didn't use DeprecationWarning, because when I tested this warn, it didn't show up when I used DeprecationWarning. It will show up as UserWarning.

@rht rht force-pushed the deprecate_neighbor_iter branch 3 times, most recently from 5573b1f to b1fab1d Compare March 8, 2022 11:31
@tpike3
Copy link
Member

tpike3 commented Mar 12, 2022

I concur this makes the methods consistent. Action (i.e. get or iter) and then target (i.e. neighbors or neighborhood).

        get_neighbors: Returns the objects surrounding a given cell.
        get_neighborhood: Returns the cells surrounding a given cell.
        iter_neighbors: Iterates over position neighbors.
        iter_neighborhood: Returns an iterator over cell coordinates that are
            in the neighborhood of a certain point.

Based on the diversity of opinions in #980 and as we are only a week away from the dev session, to ensure inclusivity we can bring up for concurrence.

Could you please squash your commits?

@rht rht force-pushed the deprecate_neighbor_iter branch from b1fab1d to bcb71ed Compare March 12, 2022 22:45
@rht
Copy link
Contributor Author

rht commented Mar 12, 2022

Squashed.

@tpike3
Copy link
Member

tpike3 commented Mar 20, 2022

@jackiekazil Can you due to the debate on this subject can you give this pull request an additional review. I concur with @rht approach.

@tpike3 tpike3 requested a review from jackiekazil March 20, 2022 09:48
@rht
Copy link
Contributor Author

rht commented Mar 20, 2022

I think this PR is not as controversial as #980. The iter_* vs get_* is not included in this PR. This PR only removes the neighbor_iter, which I haven't seen anyone arguing the case for it.

@rht
Copy link
Contributor Author

rht commented Mar 22, 2022

I'm planning to renew #835, but this work depends on getting this PR merged, because I can't cause a merge conflict by unnecessarily annotating neighbor_iter.

@jackiekazil
Copy link
Member

LGTM

@jackiekazil jackiekazil merged commit ebc6948 into projectmesa:main Mar 23, 2022
@jackiekazil
Copy link
Member

Thank you for your work on this @rht

@rht rht deleted the deprecate_neighbor_iter branch March 23, 2022 05:36
@EwoutH
Copy link
Member

EwoutH commented Sep 17, 2023

Could it be that .iter_neighbors() now returns a Coordinate (Tuple(Int, Int)) instead of an Agent? If so, is that intended behaviour with the switch from .neighbor_iter() to .iter_neighbors() in Mesa 2.0?

@rht
Copy link
Contributor Author

rht commented Sep 17, 2023

It still return agents:

return self.iter_cell_list_contents(neighborhood)
.

@EwoutH
Copy link
Member

EwoutH commented Sep 18, 2023

Ah I accidentally replaced .neighbor_iter() with .iter_neighborhood() instead of .iter_neighbors().

I also see where I went wrong, it's stated that way in the 2.0.0 release notes.

https://github.com/projectmesa/mesa/blob/7f5a32e63db93b9be2b00a705d0c28a66bc5a9dd/HISTORY.rst?plain=1#L50C13-L50C13

I opened a PR to fix this: #1807.

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

Successfully merging this pull request may close these issues.

4 participants