-
Notifications
You must be signed in to change notification settings - Fork 989
Move experimental cell spaces to normal #2286
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
Closed
Closed
Changes from all commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
d90b0f7
further updates
quaquel 9586490
Update benchmarks/WolfSheep/__init__.py
quaquel 4aaa35d
Merge remote-tracking branch 'upstream/main'
quaquel d31478c
Merge remote-tracking branch 'upstream/main'
quaquel 6e4c72e
Merge remote-tracking branch 'upstream/main'
quaquel 70fbaf5
Merge remote-tracking branch 'upstream/main'
quaquel 724c8db
Merge remote-tracking branch 'upstream/main'
quaquel 45184a4
Merge remote-tracking branch 'upstream/main'
quaquel 3d75d30
Merge remote-tracking branch 'upstream/main'
quaquel 2759244
Update __init__.py
quaquel fc8aaea
Merge remote-tracking branch 'upstream/main'
quaquel 1ba465d
Merge remote-tracking branch 'upstream/main'
quaquel 2b5e822
Merge remote-tracking branch 'upstream/main'
quaquel 3847799
Merge remote-tracking branch 'upstream/main'
quaquel 301d80e
Merge remote-tracking branch 'upstream/main'
quaquel fe3d655
Merge remote-tracking branch 'upstream/main'
quaquel 7d18880
Merge remote-tracking branch 'upstream/main'
quaquel 6b49a3b
Merge remote-tracking branch 'upstream/main'
quaquel b9909e6
Merge remote-tracking branch 'upstream/main'
quaquel 8ce3d83
Merge remote-tracking branch 'upstream/main'
quaquel 88fbf74
Merge remote-tracking branch 'upstream/main'
quaquel 077aca5
Merge remote-tracking branch 'upstream/main'
quaquel c771e3b
Merge remote-tracking branch 'upstream/main'
quaquel 3fa0c27
Merge remote-tracking branch 'upstream/main'
quaquel e99811d
move cell_space out of experimental to spaces folder
quaquel 06b0998
move boltzman benchmark wealth model to new spaces
quaquel 6e53612
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 81f7779
Update test_solara_viz.py
quaquel 72d32f4
fix benchmarks
quaquel 80a56cc
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 6697e77
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 259900e
Update cell.py
quaquel 550c123
Merge remote-tracking branch 'upstream/main' into move_spaces
quaquel 2650ad7
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,5 @@ | ||
"""Experimental init.""" | ||
|
||
from mesa.experimental import cell_space | ||
|
||
from .solara_viz import JupyterViz, Slider, SolaraViz, make_text | ||
|
||
__all__ = ["cell_space", "JupyterViz", "SolaraViz", "make_text", "Slider"] | ||
__all__ = ["JupyterViz", "SolaraViz", "make_text", "Slider"] |
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
from mesa.spaces.cell import Cell | ||
from mesa.spaces.cell_agent import CellAgent | ||
from mesa.spaces.cell_collection import CellCollection | ||
from mesa.spaces.discrete_space import DiscreteSpace | ||
from mesa.spaces.grid import ( | ||
Grid, | ||
HexGrid, | ||
OrthogonalMooreGrid, | ||
OrthogonalVonNeumannGrid, | ||
) | ||
from mesa.spaces.network import Network | ||
from mesa.spaces.voronoi import VoronoiGrid | ||
|
||
__all__ = [ | ||
"CellCollection", | ||
"Cell", | ||
"CellAgent", | ||
"DiscreteSpace", | ||
"Grid", | ||
"HexGrid", | ||
"OrthogonalMooreGrid", | ||
"OrthogonalVonNeumannGrid", | ||
"Network", | ||
"VoronoiGrid", | ||
] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Don't have a solution just yet, bit on first glance I don't directly like
OrthogonalMooreGrid
andOrthogonalVonNeumannGrid
.Maybe
OrthogonalGrid
with an required (so no default)diagonal_connections
boolean keyword argument might work. That would also make it easier to experiment with it: Instead of needing to import another space, you switch a keyword argument.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.
We debated this endlessly with the original PR. I don't want to redo that debate here. Let's first try and move all this code from experimental and take it from there in future PRs
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.
The problem is that once it's stable we can't as easily do things like this.
One part of stabilizing an API is re-evaluating properties of it. I know that will add some work, but if we're not doing that, then why did we make it experimental at all?
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.
Still you should start by revisiting the original PR.
Regarding experimentation, I don't see a lot of problem changing
spaces.OrthogonalMooreGrid
tospaces.OrthogonalVonNeumannGrid
If you import the spaces, no need to change imports.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.
To be clear, I am happy to debate the API, but there is enough stuff going on in this PR that follows from resolving all the implications that follow from moving this from experimental to stable. I feel that adding the actual API to that long list adds further confusion.
Moreover, we had good reasons in the original PR for the split and like @Corvince, I don't see a major practical difference between changing imports or a boolean.
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.
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.
So why then at the least not keep it this way? Would make migrating also easier.
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.
You normally subclass when you have either:
With diagonal/notdiagonal this is not the case, as far as I understand. They have the same methods, return the same outputs (a collection of cells) and have the same attributes (neighbours, etc.). The only thing that's different is a single function implementation (afaik), which can easily be covered by an if-else.
So why do I like keyword arguments (in this case)? Because they are more scalable. If you add another two booleans (on how many agents are allowed, another neighbour configuration, etc.), you can have 8 configuration options. With subclasses, you need to have 8 subclasses.
Here's another idea: Why not have a keyword argument
neighbour_definition
. Can be "Moore", "VonNeuman", but also something else, like a custom map. Could help a lot when we want to implement:It just feels like subclassing limits us significantly for future extension, and isn't the best practice in this case.
I'm not going to die on this hill (in this PR). Move forward with it without this if you want, I can follow up.
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 am happy to explore this further, but indeed, preferably separately.
When to subclass is a deeply contested topic, judging by the various StackOverflow questions about this that have been closed as being too opinionated. We indeed use it here for different implementations of how cells are wired up inside the grid.
But I would not say that the fact that the only difference is the wiring up method implies we should (note not could, because that is indeed possible) reduce this all to a list of options for some
neighbour_definition
keyword argument and have a long case match statement to cover all possible neighborhood definitions. From a classic OO point of view, a Moore grid, a von Neuman grid, a network, a hexgrid, or a voroinoi grid, are all discrete grids. Subclassing them while abiding by a common interface is, in fact, good design. The motivation for subclassing in this reasoning is that their behavior is different.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.
Just a note on the API (again, outside the scope of this PR): The use of subclassed to explicitly donate which space is used does not prohibit a possibly more user-friendly API. We could also think about a
create_space()
function with completely different arguments that return our spaces, without users needing to instantiate them themselves.