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

breaking: Remove visualization_old (mesa-viz-tornado) #2133

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

rht
Copy link
Contributor

@rht rht commented May 8, 2024

TODO:

@rht
Copy link
Contributor Author

rht commented May 8, 2024

This fixes the tests.

Copy link

github-actions bot commented May 8, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 -0.1% [-0.4%, +0.1%] 🔵 -0.7% [-1.0%, -0.5%]
Schelling large 🔵 -0.9% [-1.6%, -0.2%] 🔵 -2.0% [-2.9%, -1.2%]
WolfSheep small 🔵 -0.3% [-1.4%, +0.9%] 🔵 -1.1% [-1.4%, -0.8%]
WolfSheep large 🔵 -1.3% [-2.0%, -0.5%] 🔵 -2.3% [-3.9%, -0.7%]
BoidFlockers small 🔵 -0.1% [-0.8%, +0.6%] 🔵 -0.7% [-1.3%, -0.1%]
BoidFlockers large 🔵 -1.4% [-2.1%, -0.6%] 🔵 -0.3% [-1.1%, +0.3%]

@EwoutH EwoutH added the breaking Release notes label label May 8, 2024
@rht rht mentioned this pull request May 11, 2024
@rht rht force-pushed the rm_tornado branch 2 times, most recently from cfd7efa to f3e1d2f Compare May 11, 2024 11:29
@EwoutH EwoutH added this to the v3.0 milestone May 21, 2024
@EwoutH EwoutH requested review from jackiekazil and tpike3 May 29, 2024 13:24
Copy link
Contributor

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Personally, I'm going to approve this. Let's cut the cord and focus on the future. The new space needs some work (like support for Cell spaces and PropertyLayers) but it's the future.

I would like more maintainers to pitch in and/or approve (at least a second) before merging though, since this is a big change.

@EwoutH EwoutH requested a review from Corvince May 29, 2024 13:27
@jackiekazil
Copy link
Member

To be clear -- Does this do the following?

Questions --

  1. Does Remove mesa.flat namespace #2091 need to be merged first, but this shortly after?
  2. What are the recommended methods for testing this? (I looked it over, but I wan to make sure I am not missing something.)
  3. This all is consider 2.3 right, not 3.0?

@rht
Copy link
Contributor Author

rht commented Jun 24, 2024

To be clear -- Does this do the following?

It does 2 items except for adding the deprecation warning. The deprecation warning is to be added in the branch https://github.com/projectmesa/mesa/tree/2.3.x-maintenance.

Does #2091 need to be merged first, but this shortly after?

#2091 needs to be merged first, then I rebase this PR, and then it can be merged.

What are the recommended methods for testing this? (I looked it over, but I wan to make sure I am not missing something.)

The examples being tested in CI don't have frontend in them. A manual testing would be to test the Solara examples in mesa-examples.

This all is consider 2.3 right, not 3.0?

This is for 3.0.

@rht
Copy link
Contributor Author

rht commented Jun 24, 2024

A manual testing would be to test the Solara examples in mesa-examples.

Without manual changes, they will fail because they still use the mesa.experimental namespace instead of mesa.visualization.

@wang-boyu
Copy link
Member

As I mentioned in our dev meeting, my concern is about what the users need to do for this... So if a user wants to stick to tornado visualization, he/she will not be able to use Mesa's new features? Conversely if a user wants to update Mesa, he/she will need to migrate from tornado to jupyter?

I might be wrong but if this is the case, shall we have some kind of migration guide to assist the users' transition, so that they are not confused or frustrated.

@EwoutH
Copy link
Contributor

EwoutH commented Jul 2, 2024

That would be the idea (and I support it). Old visualisation use Mesa 2.3.x, new visualisation for 3.0.

We can't support everything forever, we have to move forward.

@jackiekazil
Copy link
Member

@EwoutH, this feels like a clean way to break it.

@wang-boyu
RE:

I might be wrong but if this is the case, shall we have some kind of migration guide to assist the users' transition, so that they are not confused or frustrated.

We could do a write-up for users and announce it. We need 1 or 2 use cases to create adequate documentation. Maybe we clarify that people can and are welcome to ask for help with conversions?

@EwoutH
Copy link
Contributor

EwoutH commented Jul 3, 2024

I would recommend merging this and then tag a first pre-release (Mesa 3.0.0a0), so users and developers can start testing with the new visualisation.

@EwoutH
Copy link
Contributor

EwoutH commented Jul 3, 2024

@rht
Copy link
Contributor Author

rht commented Jul 3, 2024

I have merged #2091 and rebased this PR so that it only has 1 relevant commit.

## Subpackages

```{toctree}
mesa.visualization_old
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add the new visualisation here? It's nice to have that build in the docs, even if we don't have that much docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That can happen in a separate PR. This PR is rather expensive to maintain/rebase.

@rht
Copy link
Contributor Author

rht commented Jul 3, 2024

We can use the two examples in the mesa-examples repo:

https://github.com/projectmesa/mesa-examples/tree/main/examples/boltzmann_wealth_model_experimental
https://github.com/projectmesa/mesa-examples/tree/main/examples/schelling_experimental

I think it should be either one of them, and virus on a network or boid flocker, because the 2 examples you listed have some overlap in the usage of space. Virus on a network uses network visualization, and boid flocker uses continuous space.

@EwoutH EwoutH merged commit 4cc0c82 into projectmesa:main Jul 3, 2024
10 of 11 checks passed
@rht rht deleted the rm_tornado branch July 3, 2024 11:10
@EwoutH
Copy link
Contributor

EwoutH commented Jul 3, 2024

Thanks a lot for all the work!

@wang-boyu
Copy link
Member

We could do a write-up for users and announce it. We need 1 or 2 use cases to create adequate documentation. Maybe we clarify that people can and are welcome to ask for help with conversions?

Yup for instance NetLogo has transition guide for new releases: https://ccl.northwestern.edu/netlogo/docs/transition.html

vitorfrois pushed a commit to vitorfrois/mesa that referenced this pull request Jul 15, 2024
Removing the old, legacy visualisation. It will still be available in 2.3.x.

Checkout the Visualization Tutorial (https://mesa.readthedocs.io/en/latest/tutorials/visualization_tutorial.html) to migrate to the new visualisation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants