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

ADDS ALTAIR PLOT COMPONENT AND SOME OTHER FIXES #2644

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

nissu99
Copy link
Contributor

@nissu99 nissu99 commented Jan 26, 2025

#2435

  • - rename make_space_altair to something like make_altair_space_component
  • - add make_altair_plot_component
  • - Add support for all spaces. This means including the hexgrid and network transformation of the x and y coordinates.
  • - Have a generic get_agent_data method

@quaquel @EwoutH I’ve added a new component, make_altair_plot_component. Could you kindly check if it aligns well with the project?

P.S working on space support.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 +1.3% [+0.4%, +2.2%] 🔵 -0.3% [-0.5%, -0.2%]
BoltzmannWealth large 🔵 -1.2% [-1.8%, -0.6%] 🔵 -0.5% [-1.6%, +0.7%]
Schelling small 🔵 -0.7% [-1.0%, -0.5%] 🔵 +0.8% [+0.6%, +0.9%]
Schelling large 🔵 -0.7% [-1.1%, -0.3%] 🔵 -0.9% [-1.3%, -0.5%]
WolfSheep small 🔵 +0.1% [-0.1%, +0.3%] 🔵 +1.1% [+0.9%, +1.2%]
WolfSheep large 🔵 +0.9% [+0.3%, +1.5%] 🔵 +1.9% [+1.2%, +2.5%]
BoidFlockers small 🔵 -0.7% [-1.3%, -0.2%] 🔵 +1.8% [+1.6%, +2.0%]
BoidFlockers large 🔵 -0.1% [-0.5%, +0.4%] 🔵 +1.9% [+1.6%, +2.3%]

@nissu99 nissu99 changed the title ADDS ALTAIR PLOT COMPONENT AND SOME OTHER FIXES #2435 ADDS ALTAIR PLOT COMPONENT AND SOME OTHER FIXES Jan 26, 2025
@EwoutH EwoutH requested review from Corvince and quaquel January 26, 2025 10:41
@EwoutH
Copy link
Member

EwoutH commented Jan 26, 2025

Thanks for the PR! A few of the automated checks broke, could you look into what caused that?

I will leave the review to others.

@tpike3
Copy link
Member

tpike3 commented Jan 26, 2025

@nissu99 also please look at #2643 and #2641 As you are @sanika-n are working in the same space

As it looks like you two are looking over the entire altair implementation I would also recommend looking at #2642 discussion

It is always good to collaborate and think together you may address some larger visualization challenges.

@nissu99
Copy link
Contributor Author

nissu99 commented Jan 27, 2025

@tpike3 When I run the tests locally, they don't fail, but they are failing here.

image

@tpike3
Copy link
Member

tpike3 commented Jan 27, 2025

@tpike3 When I run the tests locally, they don't fail, but they are failing here.

image

I am not sure why your tests are passing locally, however for tests to pass they need to updated. It is expecting None and now getting Chart, because you added post process.

__ ERROR collecting tests/test_components_matplotlib.py _____________
tests/test_components_matplotlib.py:[20](https://github.com/projectmesa/mesa/actions/runs/12984364297/job/36207123671?pr=2644#step:6:21): in <module>
    from mesa.visualization.mpl_space_drawing import (
mesa/visualization/__init__.py:13: in <module>
    from .components import make_plot_component, make_space_component
mesa/visualization/components/__init__.py:7: in <module>
    from .altair_components import (
mesa/visualization/components/altair_components.py:93: in <module>
    post_process: Callable[[alt.Chart], alt.Chart] | None = None,
E   AttributeError: 'NoneType' object has no attribute 'Chart'

@nissu99
Copy link
Contributor Author

nissu99 commented Jan 27, 2025

@tpike3 When I run the tests locally, they don't fail, but they are failing here.
image

I am not sure why your tests are passing locally, however for tests to pass they need to updated. It is expecting None and now getting Chart, because you added post process.

__ ERROR collecting tests/test_components_matplotlib.py _____________
tests/test_components_matplotlib.py:[20](https://github.com/projectmesa/mesa/actions/runs/12984364297/job/36207123671?pr=2644#step:6:21): in <module>
    from mesa.visualization.mpl_space_drawing import (
mesa/visualization/__init__.py:13: in <module>
    from .components import make_plot_component, make_space_component
mesa/visualization/components/__init__.py:7: in <module>
    from .altair_components import (
mesa/visualization/components/altair_components.py:93: in <module>
    post_process: Callable[[alt.Chart], alt.Chart] | None = None,
E   AttributeError: 'NoneType' object has no attribute 'Chart'

Thank You, the issue got resolved by adding altair to the toml file.

@nissu99
Copy link
Contributor Author

nissu99 commented Jan 30, 2025

@quaquel @tpike3 i was making a generic get_agent_data function ,should i remove all the old functions like _get_agent_data_new_discrete_space or keeping them will be right?

@quaquel
Copy link
Member

quaquel commented Jan 30, 2025

Removing stuff that has become redundant is fine.

@nissu99
Copy link
Contributor Author

nissu99 commented Jan 31, 2025

@quaquel i have added all the stuff,can you review it?

@quaquel
Copy link
Member

quaquel commented Jan 31, 2025

I'll try to review it over the weekend, but I am rather bussy at the moment. This is a part of the code base I am not intimately familiar with and his been a while since I used Altair, so reviewing will take more time than parts of the code base I know inside out.

@nissu99
Copy link
Contributor Author

nissu99 commented Jan 31, 2025

@tpike3 For now, there haven't been many conflicts with #2641. I have also talked to @sanika-n regarding possible overlaps ,will be looking forward for your review.

@quaquel
Copy link
Member

quaquel commented Feb 2, 2025

I suggest we try to merge this PR first, then rebase #2643 and merge that afterward. @nissu99, @sanika-n do you both agree?

My motivation is that this overhauls the structure of the altair plotting API, while the other adds a feature to the API.

@sanika-n
Copy link
Contributor

sanika-n commented Feb 3, 2025

Sure, works for me

@nissu99
Copy link
Contributor Author

nissu99 commented Feb 4, 2025

I suggest we try to merge this PR first, then rebase #2643 and merge that afterward. @nissu99, @sanika-n do you both agree?

My motivation is that this overhauls the structure of the altair plotting API, while the other adds a feature to the API.

Yes,why not.


def _get_agent_data_new_discrete_space(space: DiscreteSpace, agent_portrayal):
"""Format agent portrayal data for new-style discrete spaces.
def get_agent_data(space, agent_portrayal):
Copy link
Member

Choose a reason for hiding this comment

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

For matplotlib, we use a slightly different approach in particular for networks and hexgrids: we update the location inside the respective plotting functions.

Would it be possible to have a single overarching get_agent_data method that is valid for both matplotlib and altair?

space: the mesa.experiment.cell_space.Grid instance
agent_portrayal: the agent portrayal callable
space: Mesa space object
agent_portrayal: Function defining agent visualization properties
Copy link
Member

Choose a reason for hiding this comment

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

please check the docs for get_agent_data in mpl_space_drawing

space: the ContinuousSpace instance
agent_portrayal: the agent portrayal callable
# New DiscreteSpace
if isinstance(space, Grid):
Copy link
Member

Choose a reason for hiding this comment

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

can you rewrite this to use case switch statements? See draw_space in mpl_space_drawing.

@quaquel
Copy link
Member

quaquel commented Feb 5, 2025

Can you elaborate on the "other fixes" from the title?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants