Skip to content

Conversation

sanika-n
Copy link
Collaborator

In one of the comments present in the code review for this PR #2430 , there was a request to make some of the aliases related to the visualization module centralized, this PR aims to do that. The link can be found here
image
@EwoutH, I am not exactly sure if you meant to make it centralized throughout mesa or just in the visualization module. Since all the aliases were related to the visualization module, I have added it there.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🟢 -11.5% [-12.8%, -10.0%] 🔵 +1.1% [+0.9%, +1.2%]
BoltzmannWealth large 🔵 +0.5% [-1.2%, +3.1%] 🔵 -0.7% [-3.3%, +2.0%]
Schelling small 🔵 -0.8% [-1.2%, -0.3%] 🔵 -0.2% [-0.4%, -0.1%]
Schelling large 🔵 -0.6% [-1.1%, -0.2%] 🔵 -0.6% [-2.0%, +1.0%]
WolfSheep small 🔵 -2.3% [-2.7%, -1.9%] 🔵 +0.5% [+0.3%, +0.7%]
WolfSheep large 🔵 -0.9% [-1.7%, -0.1%] 🔵 -2.6% [-4.9%, -0.4%]
BoidFlockers small 🔵 -0.7% [-1.3%, -0.1%] 🔵 +0.5% [+0.2%, +0.7%]
BoidFlockers large 🔵 -0.9% [-1.5%, -0.3%] 🔵 +0.6% [+0.3%, +0.8%]

@quaquel
Copy link
Member

quaquel commented Jan 26, 2025

I am not sure about the need for this PR, although I realize this was suggested by @EwoutH.

Why might this not be needed

  1. old style spaces are on there way out, simplifying the typing needed
  2. Ideally, whether one uses matplotlib or altair (or plotly perhaps in the future) becomes a back-end choice. There should thus be only a single entry point for drawing spaces.

@EwoutH
Copy link
Member

EwoutH commented Jan 26, 2025

It's always good advice to check with (multiple) maintainers if something is necessary / useful / worthwhile before working on something. This can be a simple as a quick message in the Matrix chat or a follow-up comment on a PR or issue.

I don't have a strong opinion on this I will leave this to be @quaquel's call.

@EwoutH
Copy link
Member

EwoutH commented Feb 9, 2025

@quaquel do or don't?

@EwoutH
Copy link
Member

EwoutH commented Feb 16, 2025

@quaquel @Corvince @Sahil-Chhoker I'm totally indifferent on this PR, but it might be great to give @sanika-n clarity if we want to move forward with it. Especially now that we stabilized the Cell Space. Do any of you have an opinion on it?

@quaquel
Copy link
Member

quaquel commented Feb 16, 2025

I don't see much value in it at the moment.

@Sahil-Chhoker
Copy link
Collaborator

I am also with @quaquel here, and this comment gives proper explanation for that.

@sanika-n sanika-n closed this Feb 16, 2025
@EwoutH
Copy link
Member

EwoutH commented Feb 16, 2025

Thanks for the PR anyways Sakina, and sorry for adding the confusion with the issue!

@sanika-n
Copy link
Collaborator Author

No problem at all ! I should have asked before starting to work on it and anyways it was only a couple of lines of code:)

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