mesa icon indicating copy to clipboard operation
mesa copied to clipboard

examples: Rename model.grid to model.space

Open rht opened this issue 2 years ago • 9 comments

Fixes #1227.

rht avatar May 26 '22 06:05 rht

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

rht avatar May 26 '22 06:05 rht

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

tpike3 avatar May 27 '22 10:05 tpike3

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.

rht avatar May 27 '22 10:05 rht

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.

Corvince avatar May 27 '22 18:05 Corvince

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

tpike3 avatar May 28 '22 11:05 tpike3

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, ...

rht avatar May 28 '22 11:05 rht

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.

Corvince avatar May 29 '22 13:05 Corvince

The width and height could be passed explicitly, but what about https://github.com/projectmesa/mesa/blob/685494d4b022312ebeabf7dd89013b3bdd6526c2/mesa/visualization/modules/CanvasGridVisualization.py#L97 ?

rht avatar May 29 '22 13:05 rht

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

https://github.com/projectmesa/mesa/blob/685494d4b022312ebeabf7dd89013b3bdd6526c2/mesa/visualization/modules/CanvasGridVisualization.py#L97

? ( 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)

Corvince avatar May 29 '22 17:05 Corvince

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

Corvince avatar Aug 28 '23 14:08 Corvince