Skip to content
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

Integrate Jupyterviz #199

Closed
wants to merge 0 commits into from
Closed

Integrate Jupyterviz #199

wants to merge 0 commits into from

Conversation

tpike3
Copy link
Member

@tpike3 tpike3 commented Apr 7, 2024

Unfortunately with my current job, I am always behind so this is not very clean or optimized however this integrates jupyter_viz into mesa_geo

Please also see mesa-examples pull request for browser and jupyter examples

TODO: fix the jupyter version when you try and expand from the cell output to full screen

@tpike3 tpike3 requested a review from wang-boyu April 7, 2024 13:13
Copy link

codecov bot commented Apr 7, 2024

Codecov Report

Attention: Patch coverage is 0.68027% with 146 lines in your changes are missing coverage. Please review.

Project coverage is 64.34%. Comparing base (ce501b6) to head (699f1ac).
Report is 5 commits behind head on main.

❗ Current head 699f1ac differs from pull request most recent head f3b4cd2. Consider uploading reports for the commit f3b4cd2 to get more accurate results

Files Patch % Lines
mesa_geo/geoexperimental/geojupyter_viz.py 0.00% 75 Missing and 2 partials ⚠️
mesa_geo/geoexperimental/leaflet_viz.py 0.00% 67 Missing ⚠️
mesa_geo/geoexperimental/__init__.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #199       +/-   ##
===========================================
- Coverage   78.21%   64.34%   -13.87%     
===========================================
  Files          10       13        +3     
  Lines         693      833      +140     
  Branches      151      186       +35     
===========================================
- Hits          542      536        -6     
- Misses        127      268      +141     
- Partials       24       29        +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rht
Copy link
Contributor

rht commented Apr 8, 2024

The code shouldn't be duplicated as much as possible, otherwise it is much more difficult to maintain.

@tpike3
Copy link
Member Author

tpike3 commented Apr 8, 2024

The code shouldn't be duplicated as much as possible, otherwise it is much more difficult to maintain.

Removed the draw_grid code from components as the assumption is it will not be used since users will put everything on maps.

Are you also saying it should import more from Mesa (e.g. the matplotlib)?

@wang-boyu wang-boyu requested review from rht and Corvince April 8, 2024 15:32
@@ -0,0 +1,56 @@
class UserParam:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file can be removed and to use the Slider, you can import from mesa.experimental.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep got it.. in retrospect it seem obvious, ahh well such is life

@@ -0,0 +1,410 @@
import sys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forking the entire file here is not sustainable. Needs to do it the way it is done here https://github.com/projectmesa/mesa-examples/blob/1933cbc8251ca047273e0f88193f692ccbb86de6/examples/sugarscape_g1mt/app.py#L9.

@tpike3
Copy link
Member Author

tpike3 commented Apr 9, 2024

Still rushing through this but let me know what else I need to do so we can get this release before AAG.

Sorry for the hassle

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.

2 participants