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

Convert make_space into solara components #1789

Closed
wants to merge 2 commits into from
Closed

Conversation

Corvince
Copy link
Contributor

@Corvince Corvince commented Sep 3, 2023

In this PR I wanted to convert the make_space function into a SpaceView component that handles all the aspects of drawing the model space.

However, as it is now, it doesn't work, because in contrast to React solara uses lazy rendering. That is child component only rerender if their props change. But with SpaceView I am only passing space_drawer, model and agent_portrayal. And those don't change with each step. I found two workarounds

  1. pass current_step as additional prop. This works but then it needs to be passed again to SpaceViews child components as well
  2. remove @solara.component from SpaceView and its children. Thus converting them to normal functions that return solara components.

Both approaches seems rather hacky. Maybe someone else has any ideas?

@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

Patch coverage: 19.35% and project coverage change: -0.45% ⚠️

Comparison is base (47637a7) 79.88% compared to head (4aa02fd) 79.43%.

❗ Current head 4aa02fd differs from pull request most recent head 6d73151. Consider uploading reports for the commit 6d73151 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1789      +/-   ##
==========================================
- Coverage   79.88%   79.43%   -0.45%     
==========================================
  Files          15       15              
  Lines         880      890      +10     
  Branches      188      192       +4     
==========================================
+ Hits          703      707       +4     
- Misses        154      160       +6     
  Partials       23       23              
Files Changed Coverage Δ
mesa/experimental/jupyter_viz.py 20.13% <19.35%> (+1.48%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rht
Copy link
Contributor

rht commented Sep 4, 2023

This is how Solara does it: https://github.com/widgetti/solara/blob/baa36623c3eb7db50672d8eb3d3cdab9220a50a6/solara/components/matplotlib.py#L11, add an optional argument dependencies, and the decorator will automatically handle it. Haven't tested whether it has to be passed recursively or not.

@Corvince
Copy link
Contributor Author

Corvince commented Sep 4, 2023

This is how Solara does it: https://github.com/widgetti/solara/blob/baa36623c3eb7db50672d8eb3d3cdab9220a50a6/solara/components/matplotlib.py#L11, add an optional argument dependencies, and the decorator will automatically handle it. Haven't tested whether it has to be passed recursively or not.

No, this is the opposite. This is used to limit the number of re-renders, where I want to trigger a re-render. I mean the idea of passing and additional, changing prop works and this is what my solution 1) refers to (and what i got recommended on discord). But the dependencies argument in Solaras FigureMatplotlib in particular is meant to do the opposite.

@Corvince
Copy link
Contributor Author

Corvince commented Sep 4, 2023

Added the additional prop for now - still not completely happy with this solution but at least it works right now

@Corvince Corvince marked this pull request as ready for review September 5, 2023 07:27
def make_space(model, agent_portrayal):
def portray(g):
@solara.component
def GridView(grid, agent_portrayal, dummy):
Copy link
Contributor

Choose a reason for hiding this comment

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

dummy as a name is rather non-descriptive and confusing.

@Corvince Corvince closed this Sep 27, 2023
@Corvince Corvince deleted the solara-space-view branch September 27, 2023 19:07
@Corvince
Copy link
Contributor Author

Reason to close this PR #1805 (comment)

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

Successfully merging this pull request may close these issues.

2 participants