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

fix: Explicitly specify JupyterViz space view limits #1984

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

rht
Copy link
Contributor

@rht rht commented Jan 21, 2024

Fixes #1977. Manually tested on the Schelling (regular 2D grid) and Boid flocker (continuous) example.

@@ -55,6 +55,8 @@ def portray(g):
out["c"] = c
return out

space_ax.set_xlim(-1, space.width)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has to start at -1, because otherwise the markers at position 0 are cropped out.

Copy link
Member

@quaquel quaquel Jan 21, 2024

Choose a reason for hiding this comment

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

Probably go with space.width+1 as well for the same reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

space.width+1 is not necessary, because the position is 0-index based, so technically it already has +1 in it by doing space.width instead of space.width-1.

@quaquel
Copy link
Member

quaquel commented Jan 21, 2024

Code looks good to me.

@EwoutH
Copy link
Member

EwoutH commented Jan 21, 2024

For clarity, could you add a before and after screenshot?

@rht
Copy link
Contributor Author

rht commented Jan 21, 2024

There is no perceptible change in the before and after screenshot for both the Schelling and Boid flocker, and hence why I didn't upload anything. However, I have tested the negative cases by intentionally setting the xlim/ylim to (-1, 2), and the displayed scatter plots are cut off as expected. It's standard Matplotlib.

@quaquel
Copy link
Member

quaquel commented Jan 21, 2024

For clarity, could you add a before and after screenshot?

I am making an exam model with a random walk in continuous space. Here this makes a big difference. If I have time, I'll try to make a short movie.

@quaquel
Copy link
Member

quaquel commented Jan 22, 2024

old behavior:
Screenshot 2024-01-22 at 10 37 52

new behavior
Screenshot 2024-01-22 at 10 37 20

@EwoutH
Copy link
Member

EwoutH commented Jan 22, 2024

@quaquel Thanks! I have difficulty seeing what I should notice (as differences) here.

@rht
Copy link
Contributor Author

rht commented Jan 22, 2024

The plot on the right side is a guide of how far the positions should be relative to the [0, 1] range. As in, they shouldn't be too close to the top/right edge of the space.
Edit: s/should/shouldn't/

@EwoutH
Copy link
Member

EwoutH commented Jan 22, 2024

Right, it's about the blue dots representing the agents. Why isn't anything like a grid, axis or canvas visible that represents the space? Now the agents just sit there in total isolation.

@rht
Copy link
Contributor Author

rht commented Jan 22, 2024

Why isn't anything like a grid, axis or canvas visible that represents the space? Now the agents just sit there in total isolation.

A grid is misleading, because then it would look like a discrete grid. But a box border or axis would make sense.

@EwoutH
Copy link
Member

EwoutH commented Jan 22, 2024

What about a light grey background representing the total space? With maybe a hard black border if torus=False and no border (or a dotted one?) if torus=True.

@rht
Copy link
Contributor Author

rht commented Jan 22, 2024

I think it is out of scope for this PR, but we can continue discussing after the PR has been merged.

@EwoutH
Copy link
Member

EwoutH commented Jan 22, 2024

@quaquel you have validated this fixes your problem, without know side-effects or breaking changes as of now?

If so, @rht go ahead and merge.

@quaquel
Copy link
Member

quaquel commented Jan 22, 2024

@EwoutH, I can more easily show you in person. The current problem is that ax.scatter autoscales (default matplotlib behavior), so when you have agents moving in space, you don't properly see their movement because the view limits change each step. You see the movement properly by fixing the view limits based on the space limit. And yes, this fix works as intended and no side effects (it really is just a few matplotlib commands, don't overthink this).

I personally would like to see just the spines of the axes and, in the case of a continuous grid, which is my example, perhaps some ticks. But I agree with @rht; this is probably beyond this simple PR, which is really just a bug fix.

@EwoutH
Copy link
Member

EwoutH commented Jan 22, 2024

so when you have agents moving in space, you don't properly see their movement because the view limits change each step.

I now understand the problem. @rht go ahead with merging if you want.

I will try a simple PR for a background color and some axis.

@rht rht merged commit 665aebc into projectmesa:main Jan 22, 2024
13 checks passed
@rht rht deleted the jupyterviz_limit branch January 22, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Release notes label visualisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JupyterViz space view limits
4 participants