Skip to content

Conversation

@rht
Copy link
Contributor

@rht rht commented May 26, 2022

Fixes #1227.

@rht rht marked this pull request as ready for review May 26, 2022 06:38
@rht
Copy link
Contributor Author

rht commented May 26, 2022

Ugh, this is actually a breaking change, because HexGridVisualization and CanvasGridVisualization assume model.grid. Should we introduce this breaking change?

@tpike3
Copy link
Member

tpike3 commented May 27, 2022

Ugh, this is actually a breaking change, because HexGridVisualization and CanvasGridVisualization assume model.grid. Should we introduce this breaking change?

This is a pretty big one where I think the majority of user visualizations will break. I started working through some work arounds but none were suitable, so will think more on it.

Broad strokes I think there is three paths:

  • we do it
  • we don't do it
  • we keep grid for the examples in HexGridVisualization and CanvasGridVisualization as they are grids and change the others which are not grid

@rht
Copy link
Contributor Author

rht commented May 27, 2022

There is always room for adding an informative error message, at the right time. In the 2 classes, we check if users have model.grid defined. If so, we raise an exception telling what is wrong, and recommend people to use model.space.

@Corvince
Copy link
Contributor

I think this is not worth a breaking change. I think we should either

  1. Do what @rht said, but with a warning or
  2. Allow both .grid and .space, but think about how to deal with this in the future.

The reason for 2 is that I think depending on an implicit naming scheme is bad practice. We should either somehow enforce a name or leave the choice completely open to the users.

@tpike3
Copy link
Member

tpike3 commented May 28, 2022

I think this is not worth a breaking change. I think we should either

  1. Do what @rht said, but with a warning or
  2. Allow both .grid and .space, but think about how to deal with this in the future.

The reason for 2 is that I think depending on an implicit naming scheme is bad practice. We should either somehow enforce a name or leave the choice completely open to the users.

Concur, and I vote 2 as we have already had issues with implicit naming and my bias would be to leaving the choice to the users

@rht
Copy link
Contributor Author

rht commented May 28, 2022

We sidestepped the model.datacollector naming by introducing model.initialize_datacollector (which does initial data collection under the hood, where people often forget to do). But we can't do the same with space/grid, because with datacollector, >90% of the case it is mesa.DataCollector, but with space, it could be SingleGrid, MultiGrid, NetworkGrid, ...

@Corvince
Copy link
Contributor

We could just add space as an additional function parameter to the modules. CanvasGrid currently takes model width and space, but doesn't actually use them, don't know how that evolved. But I think this is a clean solution and should even he doable without breakage.

@rht
Copy link
Contributor Author

rht commented May 29, 2022

The width and height could be passed explicitly, but what about

cell_objects = model.grid.get_cell_list_contents([(x, y)])
?

@Corvince
Copy link
Contributor

The width and height could be passed explicitly, but what about

cell_objects = model.grid.get_cell_list_contents([(x, y)])

?
(
Sorry I realised my comment was a bit confusing. I would just add a space parameter (with a space instance) and derive everything needed from that. The width/height is just an oddity i saw when analysing CancasGrid.
Just to be clear, a complete example might look something like this

mycanvas = CanvasGrid(grid=mymodel.space, canvas_width=500,  canvas_height=500)

@tpike3 tpike3 added this to the Mesa 2.0 milestone Jan 8, 2023
@tpike3 tpike3 removed this from the Mesa 2.0 milestone Jun 18, 2023
@Corvince
Copy link
Contributor

Since the examples are now in their own repo this can be closed

@Corvince Corvince closed this Aug 28, 2023
@rht rht deleted the grid2space branch August 28, 2023 16:13
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.

code convention: Rename model.grid attribute to model.space

3 participants