remove __geo_interface__ functions

[wang-boyu]

1. Fix Exclude some GeoAgents attributes from visualization #32
2. Change model.grid to model.space for better naming convention, since GeoSpace is not necessarily a grid.

[rht]

Change model.grid to model.space for better naming convention, since GeoSpace is not necessarily a grid.

Make sense. I'm up for attribute names in projectmesa/mesa to be renamed to space as well.

[rht]

You should resolve the merge conflict by pull --rebase on top of current master.

[wang-boyu]

Done. (But this rewrites the history and need to force a push.)

[rht]

Would it make sense for mapping(transform(model.space.Transformer.transform, agent.shape)) to be wrapped as a method of GeoAgent, maybe get_geometry?

[wang-boyu]

This is a good point, although agent.get_geometry() will probably just return agent.shape. model.space.Transformer then transforms agents' coordinates to be consistent with Leaflet's default epsg:4326 crs. (But why does GeoSpace know the default crs of MapModule and do the transformation?)

The current crs settings in mesa-geo are a bit convoluted and inconsistent (for example #20). I'll raise an issue regarding this.

[rht]

mapping(transform(model.space.Transformer.transform, agent.shape)) is way too long, and so I think it would be helpful to package it in a method. Maybe call it instead agent.get_transformed_geometry?

[wang-boyu]

Yup this looks better. Added the new function.

This makes me wonder whether agent.shape should be renamed to agent.geometry to be consistent with the GeoDataFrame (as mentioned in #37). But we can do this in a separate PR.

[rht]

Make sure to not make another PR after this one, unless it is for flake8 and Black fixes. Otherwise, the merge conflicts would be nasty.

[rht]

By definition, the output of __geo_interface__ seems to be JSON-serializable. If it is not, then the problem is in the implementation. Are you sure this is not needed elsewhere? You could at least just not use __geo_interface__ in the visualization render(), without deleting this method. I haven't used Mesa-Geo much to be able to tell which should be the case.

[wang-boyu]

It was used in the MapModule.render() function, and the GeoSpace.__geo_interface__ function, which was not used and similarly deleted. I guess it's a similar idea from geopandas.GeoDataFrame.__geo_interface__

There're three problems if we implement this method properly:

1. By definition all attributes of an agent should be json serialised. Each agent has a model attribute, i.e., agent.model, which is currently serialised as str(self.model)(but why?). Plus, the users need to make sure that all additional fields in their self-defined GeoAgent are json serialisable.
2. Not all attributes are necessary for frontend visualisation. This links to Exclude some GeoAgents attributes from visualization #32 and is the motivation of this PR. The necessary properties for visualisation are set by MapModule.portrayal_method() which is outside GeoAgent.
3. This method is not used anywhere else apart from MapModule.render().

Options:

1. We fix it, and keep it. This might leads to further issues from users (or not).
2. We do nothing and keep it. Again this might leads to further issues from users (or not).
3. We delete it, bring it back in the future if needed, and fix it then.

Do you have any suggestions?

[rht]

self.model is probably a complex beast, and hence why only the str of it becomes its serialized version.
From reading that geopandas.org documentation, I see that __geo_interface__ is needed for GeoJSON serialization in the future, and hence, not necessarily for visualization. We might want the simulation state (which includes the agents) to be serializable, so that it can be resumed at a different time, on a different machine.

[rht]

To keep this PR small and hopefully mergeable soon, I think we should just do nothing about __geo_interface__ for now. We can delete/fix it in a separate PR.

[wang-boyu]

Sure, I've added them back.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@75d356e). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 3c454ab differs from pull request most recent head 64e1676. Consider uploading reports for the commit 64e1676 to get more accurate results

@@            Coverage Diff            @@
##             master      #40   +/-   ##
=========================================
  Coverage          ?   49.00%           
=========================================
  Files             ?        5           
  Lines             ?      302           
  Branches          ?        0           
=========================================
  Hits              ?      148           
  Misses            ?      154           
  Partials          ?        0           

---

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 75d356e...64e1676. Read the comment docs.

[rht]

OK, the last thing is if you could squash the 4th commit into the 1st commit via the interactive rebase, since they logically are within the same theme.

[wang-boyu]

Done!