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

@Sahil-Chhoker
Copy link
Collaborator

You've done a great job @nissu99 on the grids. It will take some time to test them all but I will do a review soon enough.

Copy link
Collaborator

@Sahil-Chhoker Sahil-Chhoker left a comment

Choose a reason for hiding this comment

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

I have done a basic review, the grid drawing can wait, let's go step by step.
Our first step here should be to finalize the agent data collecting logic, therefore why is the agent data collecting logic not made into a function like is done in matplotlib part?
Also does this implementation cover all the grids (again compare with this)

Comment on lines +16 to +17
with contextlib.suppress(ImportError):
import altair as alt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't suppress the import error here.

Comment on lines 13 to 14
import mesa.experimental
from mesa.experimental.cell_space import HexGrid
Copy link
Collaborator

Choose a reason for hiding this comment

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

change the experimental imports to mesa.discrete_space

visualized. The dictionary can contain the following keys:
- "color": A string representing the agent's color (e.g., "red", "#FF0000").
- "size": A number representing the agent's size.
- "tooltip": A string to display as a tooltip when hovering over the agent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this tooltip a new feature for altair? I don't think this is necessary for the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is used for displaying metadata.

update_counter.get()
space = getattr(model, "grid", None)
if space is None:
# Sometimes the space is defined as model.space instead of model.grid
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should keep this comment.

space = model.space

chart = _draw_grid(space, agent_portrayal)
# Apply post-processing if provided
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should keep this comment as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, will be more cautious while deleting comments.

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 2, 2025

I have done a basic review, the grid drawing can wait, let's go step by step. Our first step here should be to finalize the agent data collecting logic, therefore why is the agent data collecting logic not made into a function like is done in matplotlib part? Also does this implementation cover all the grids (again compare with this)

stuff like vernoi grid has not been added ,will have to look into it to properly add it.

@Sahil-Chhoker
Copy link
Collaborator

@nissu99 I can see why you got confused, I think he meant the generic function should be called in the respective draw functions and hence agnet_portrayal was passed to the draw functions. Again if you look at the matplotlib side you will get it. I think the exact same method can be used to get the data, can you try importing the method from the matplotlib side and using it to test if that works?

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 2, 2025

@nissu99 I can see why you got confused, I think he meant the generic function should be called in the respective draw functions and hence agnet_portrayal was passed to the draw functions. Again if you look at the matplotlib side you will get it. I think the exact same method can be used to get the data, can you try importing the method from the matplotlib side and using it to test if that works

I thought ,later he wants to make some universal function which can be reused.

@Sahil-Chhoker
Copy link
Collaborator

You don't have to worry about the new API, its still in discussion.

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 2, 2025

You don't have to worry about the new API, its still in discussion.

i tried using the collect_agent_data function it does not give the desired results.

@Sahil-Chhoker
Copy link
Collaborator

How did you test it, can you please elaborate how is it not working?

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 2, 2025

will ha

How did you test it, can you please elaborate how is it not working?

i used the internal examples ,somehow it shows no agent showing ,it is not able to extract the data ,will have to properly look into it to understand the issue well.

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 2, 2025

image

@Sahil-Chhoker
Copy link
Collaborator

Sahil-Chhoker commented Mar 2, 2025

That's because you are just getting the agent data, no plotting them on the grid. Maybe check if the data extraction is working or not don't not rely on this visualization.

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 8, 2025

@Sahil-Chhoker sorry for not getting back ,just got my college holidays started , i think the extraction is the fault here
when i printed the collect_agent_data returned values it was an empty array .

@Sahil-Chhoker
Copy link
Collaborator

Its working fine for me, can you test again. I don't think its a difficult task, you just replace the agent data collection logic with the collect_agent_data function call inside the grid draw logic.

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 8, 2025

Its working fine for me, can you test again. I don't think its a difficult task, you just replace the agent data collection logic with the collect_agent_data function call inside the grid draw logic.

i get that , it is not working for OrthogonalMooreGrid somehow,for others it works fine.

@Sahil-Chhoker
Copy link
Collaborator

It's working for OrthogonalMooreGrid as well. Please review your code carefully before commenting, as I may not have the capacity to review it multiple times.

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 9, 2025

It worked ,i made a dumb mistake ,should i make a commit for changing the extraction logic?

@Sahil-Chhoker
Copy link
Collaborator

Yeah, refactor the agent placement logic inline with the extraction logic.

@Sahil-Chhoker
Copy link
Collaborator

I encountered several issues while reviewing this PR. First, the previous review comments have not been addressed. Additionally, you're not using the new grids in mesa.discrete_space, among other issues.

Due to the excessive length of this PR, it's difficult for me to review it entirely. I strongly recommend breaking it down into smaller, atomic PRs that address specific issues.

A better approach could be:

  1. Implement rectangular grids with agent placement logic.
  2. Implement hex grids with agent placement logic.
  3. Implement continuous space with agent placement logic.
  4. Implement Voronoi grids with agent placement logic.
  5. Implement network grids with agent placement logic.
  6. Address any remaining grid implementations.
  7. Add the plotting function.

This will make the review process smoother and ensure that changes are properly implemented step by step.

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 11, 2025

I will change all the experimental to discrete_space ,I agree the changes have increased and i understand it is becoming very hard to review , is the extraction logic fine ?

@Sahil-Chhoker
Copy link
Collaborator

I have only tested it once, so it may be a good starting point. And again I strongly recommend you to break this PR into smaller once.

@Corvince
Copy link
Contributor

And again I strongly recommend you to break this PR into smaller once.

Maybe to clarify what this means: Just open a series of new PRs, one after another. So start with some minimal changes as a single PR, which includes the groundwork for the rest. Once this PR is merged, open the next PR with changes to some other function/grid/whatever. And so on. Its more tedious this way, but overall it will speed things up. Right now the PR is too big to properly review, because it requires code review, which is already almost too much, but also manually testing everything.

You can also just leave this PR open for now as a reference.

@Sahil-Chhoker
Copy link
Collaborator

Thanks for the clarification @Corvince!

@nissu99
Copy link
Contributor Author

nissu99 commented Mar 12, 2025

And again I strongly recommend you to break this PR into smaller once.

Maybe to clarify what this means: Just open a series of new PRs, one after another. So start with some minimal changes as a single PR, which includes the groundwork for the rest. Once this PR is merged, open the next PR with changes to some other function/grid/whatever. And so on. Its more tedious this way, but overall it will speed things up. Right now the PR is too big to properly review, because it requires code review, which is already almost too much, but also manually testing everything.

You can also just leave this PR open for now as a reference.

Thank You for clarity ,will be making individual PR's .

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.

7 participants