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

breaking: NetworkGrid: modify get_neighbors and create get_neighborhood #1542

Merged
merged 1 commit into from
Jun 18, 2023

Conversation

Tortar
Copy link
Contributor

@Tortar Tortar commented Dec 2, 2022

NetworkGrid didn't have a get_neighbors function which returns the adjacent agents before this PR, but I think it's an important part of the API, unfortunately it has already a get_neighbors function which returns ids, but clearly changing it would be a breaking change.

I think if we want to break it in 2.0 we could rename get_neighbors to get_neighborhood and rename this function to get_neighbors. Is it better to include this PR now as it is and make a separate PR with the breaking change or modify this one with the proposed changes?

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Patch coverage: 100.00% and project coverage change: +10.77 🎉

Comparison is base (2a50370) 80.52% compared to head (8effe7e) 91.30%.

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

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1542       +/-   ##
===========================================
+ Coverage   80.52%   91.30%   +10.77%     
===========================================
  Files          18       15        -3     
  Lines        1248     1311       +63     
  Branches      240      229       -11     
===========================================
+ Hits         1005     1197      +192     
+ Misses        205       80      -125     
+ Partials       38       34        -4     
Impacted Files Coverage Δ
mesa/space.py 95.82% <100.00%> (+4.57%) ⬆️

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rht
Copy link
Contributor

rht commented Dec 2, 2022

It would be less confusing to do this all at once for 2.0, instead of defining a new temporary API get_agent_neighbors, only for it to be renamed. I have updated #1511 to include this PR.

@Tortar Tortar changed the title Create NetworkGrid.get_agents_neighbors Create NetworkGrid.get_neighbors Dec 2, 2022
@Tortar Tortar changed the title Create NetworkGrid.get_neighbors NetworkGrid: modify get_neighbors and create get_neighborhood Dec 2, 2022
@Tortar
Copy link
Contributor Author

Tortar commented Dec 2, 2022

ok @rht, not it should be ready to be merged in 2.0

@rht rht added the breaking Release notes label label Dec 8, 2022
@tpike3 tpike3 added this to the Mesa 2.0 milestone Jan 7, 2023
@rht rht changed the title NetworkGrid: modify get_neighbors and create get_neighborhood [BREAKING] NetworkGrid: modify get_neighbors and create get_neighborhood Jun 18, 2023
@rht rht changed the title [BREAKING] NetworkGrid: modify get_neighbors and create get_neighborhood breaking: NetworkGrid: modify get_neighbors and create get_neighborhood Jun 18, 2023
@rht
Copy link
Contributor

rht commented Jun 18, 2023

I have rebased the PR, resolving the merge conflicts, and create projectmesa/mesa-examples#40 for the examples update. Merging; thank you @Tortar.

@rht rht merged commit 82fc3c5 into projectmesa:main Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants