-
Notifications
You must be signed in to change notification settings - Fork 58
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
add method to make geospace as a solara component #246
Conversation
Great work! Looks pretty good from a brief look, but as said earlier can't test it atm |
92abfdd
to
4184442
Compare
I will make time for full review Monday, but the fact that this replaces 428 lines of code with only 84 is always a good sign! |
Looks like an awesome, almost drop-in replacement! Could we add aliases the old names at some place to keep those working (maybe with a deprecation warning to use the new name)? Is there anything in particular you would like me to review or test in detail? |
Thanks for your comments!
Certainly! I've added another item into the todo list. Will bring back
Mostly about how Thanks agin for looking into this. |
4184442
to
04ffc07
Compare
Rebased to main branch |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #246 +/- ##
==========================================
- Coverage 78.21% 73.48% -4.73%
==========================================
Files 10 10
Lines 693 856 +163
Branches 151 182 +31
==========================================
+ Hits 542 629 +87
- Misses 127 194 +67
- Partials 24 33 +9 ☔ View full report in Codecov by Sentry. |
Nice! Test pass, that's a good thing. Let's us know if you need any input! |
04ffc07
to
310299d
Compare
Brought back Ready for merging now. |
Any specific input/testing/review you need? |
Thanks @EwoutH. I experienced some slowness while trying the new solara api (also reported here projectmesa/mesa#2289 (comment)), for both this intro tutorial as well as Mesa's viz tutorial. Would be great if you could confirm this (or it could be just my machine). But I think this is outside the scope of this PR, since |
@tpike3 this needs your review. |
This PR has been stuck for a while, and given that @tpike3 hasn't responded, I am going to unblock by approving. |
@wang-boyu test GIS examples are still failing. |
I resolved almost all errors and warnings in the GIS examples (only one left is projectmesa/mesa-examples#209). |
I would love to do a Mesa-geo pre-release this week, since now all the gis examples work with the latest Mesa 3.0 pre-release. It would be really nice to have this in! |
310299d
to
bcb7ea1
Compare
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.
Let’s give @tpike3 a few more hours, most of the time he’s online between 10 and 12 CET.
Thanks for reviewing, did you try it yourself?
That's not related to this PR, and we already fixed that in #251 |
Okay so I tested this by building a viz app for geo_sir import sys
import os
# Add the directory containing mesa-geo to the Python path
sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', '..')))
from mesa.visualization import SolaraViz, make_plot_measure
from agents import PersonAgent
from model import GeoSir
# Import the make_geospace_leaflet function directly
from mesa_geo.visualization import make_geospace_leaflet
def SIR_draw(agent):
"""
Portrayal Method for canvas
"""
portrayal = {}
if isinstance(agent, PersonAgent):
portrayal["marker_type"] = "Circle"
portrayal["radius"] = 2
if agent.atype in ["hotspot", "infected"]:
portrayal["color"] = "Red"
elif agent.atype in ["safe", "susceptible"]:
portrayal["color"] = "Green"
elif agent.atype in ["recovered"]:
portrayal["color"] = "Blue"
elif agent.atype in ["dead"]:
portrayal["color"] = "Black"
return portrayal
model_params = {
"pop_size": {
"type": "SliderInt",
"value": 30,
"min": 10,
"max": 100,
"step": 10,
"label": "Population size",
},
"init_infected": {
"type": "SliderFloat",
"value": 0.2,
"min": 0.0,
"max": 1.0,
"step": 0.05,
"label": "Fraction initial infection",
},
"exposure_distance": {
"type": "SliderInt",
"value": 500,
"min": 100,
"max": 1000,
"step": 100,
"label": "Exposure distance",
},
}
model1 = GeoSir()
page = SolaraViz(
model1,
model_params=model_params,
name="Basic agent-based SIR model",
components=[
make_geospace_leaflet(SIR_draw, zoom=12, scroll_wheel_zoom=False),
make_plot_measure(["infected", "susceptible", "recovered", "dead"]),
make_plot_measure(["safe", "hotspot"]),
]
) Running with:
(yeah I named my folder terribly) It runs, which is great:
|
OK thats because mesa-geo assigns a very large number to plygon agents, which should now be replaced just the auto created id. I am looking for the line(s) |
@EwoutH I think the problem is here --mesa-geo agents property This is using agents as a property and may be causing a collision with agents in Mesa now. Mesa-geo creates a rtree index for spatial indexing, so I think the agent_ids are coming into conflict. |
Is it because of this: Line 322 in 4e22c5c
We used the agent object id as key, and the agent object may be deleted and recreated during model reset, resulting in a different object id. Changing this to |
Replaced |
Awesome, resetting the model now works! |
I'm good with merging. It works, run well, and it's great that it now uses SolaraViz directly, it's nice to have a single interface. It might even help with projectmesa/mesa#2330 ;) Thanks for the effort! |
Congrats! Are there any old viz components that should be removed or deprecated? |
No I would prefer to keep the current |
This PR:
GeoJupyterViz
andLeafletViz
, hence partly addresses Integrate Jupyterviz #199 (comment) by removing code duplicated from Mesamake_geospace_leaflet
for the new SolaraViz api in Mesa, as previously discussed in:As a result, instead of
we can now do
A few other things to consider (could be separate PRs, or to be addressed in this one):
update intro tutorial(in separate PR)have a default geospace component, similar to add default SolaraViz mesa#2280 for Mesa(in separate PR)wait for a new name for SolaraViz(doesn't really matter; only need to update tutorial)GeoJupyterViz
with deprecation warnings@tpike3 would you like to test this out and see if it's working as expected?