UI layout by ankitk50 · Pull Request #1825 · mesa/mesa

ankitk50 · 2023-09-26T15:13:18Z

This PR will focus on changes to the UI layout. Below is the initial proposal.

Corvince · 2023-09-26T17:01:39Z

Ha, funny, I was experimenting today with the exact same ideas. The only change I would suggest is putting the controls into the main area and not the sidebar. On small screens and thus often in Notebooks the sidebar is collapsed by default and doesn't stay open, so this makes it hard to control the model.

But otherwise this is very nice 👍

Corvince · 2023-09-26T17:03:34Z

mesa/experimental/jupyter_viz.py

-    solara.Markdown(name)
-    UserInputs(user_params, on_change=handle_change_model_params)
-    ModelController(model, play_interval, current_step, set_current_step, reset_counter)
+    with solara.AppBarTitle():


Suggested change

with solara.AppBarTitle():

solara.Title(name)

This also sets the tab title

ankitk50 · 2023-10-03T11:29:16Z

I have added a check that allows us to identify Jupyter from normal. And based on this we can change the UI layout.

rht · 2023-10-07T08:09:13Z

mesa/experimental/jupyter_viz.py


 # Avoid interactive backend
 plt.switch_backend("agg")
+plt.rcParams["figure.figsize"] = (7, 7)


You shouldn't set the figure size globally, because this affects both the space/grid and the plots. Needs to be more fine-grained. This comments applies to #1820 as well.

Will resolve this by tomorrow, thanks for pointing this out

I think I need an explanation on why this hasn't been acted on in 3 weeks.

this has been removed

I know. It took 3 weeks. What is the explanation?

at the moment -- No.

I'm aware of #1821, it's just that I feel it would be much easier to build upon when #1825 merged and stable.

Same with #1820. It will become irrelevant after #1825 is merged. Scaling happens automatically.

#1821 is simple to implement, orthogonal to your existing 2 PRs. Waiting for #1825 is not a sufficient justification for not working on it. If you don't want to do it, please say sooner. It is a needed feature that needs to be delivered soon. I will just do it.

#1820 is needed because this PR only handles the total space visualization with, and doesn't fix the overlapping markers.

I am appalled by your responses here so far. No replies on #1820, then suddenly you decided to not work on it for a reason that you think it is irrelevant, and which it is not justifiable either.

I am very sorry for my comments and the lack of understanding of how you allocate time for your workflow, given that you are busy with classes as well. It was uncalled for. I haven't said earlier that you should have communicated your thoughts that #1820 is unnecessary. But it would be great next time to tell this and so we can discuss about it. Thank you for what you have done so far, and hope we can get this done by CSSSA.

codecov · 2023-10-30T00:00:58Z

Codecov Report

Attention: 61 lines in your changes are missing coverage. Please review.

Comparison is base (adc5549) 79.12% compared to head (c464a33) 78.27%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1825      +/-   ##
==========================================
- Coverage   79.12%   78.27%   -0.86%     
==========================================
  Files          15       17       +2     
  Lines         915      962      +47     
  Branches      194      214      +20     
==========================================
+ Hits          724      753      +29     
- Misses        168      183      +15     
- Partials       23       26       +3

Files	Coverage Δ
mesa/experimental/user_input.py	`72.22% <72.22%> (ø)`
mesa/experimental/jupyter_viz.py	`38.69% <62.50%> (+8.52%)`	⬆️
mesa/experimental/model_control.py	`12.50% <12.50%> (ø)`

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

tpike3 · 2023-10-30T00:03:49Z

@ankitk50 Thanks, very excited about these updates. To merge can you:

take a look at your lint and ruff failures please.
Also take a look at your commits and see if you can group/squash them into the key changes. The goal is a easily readable where the key changes are easily identifiable.

Again very cool. Excited to get this into mesa.

mesa/experimental/jupyter_viz.py

rht · 2023-10-30T03:57:40Z

mesa/experimental/jupyter_viz.py

+
+    # render layout and plot
+
+    if "ipykernel" in sys.argv[0]:  # jupyter


Can you split the following if-else into 2 components each defined in a function?

component forces re-render of Cards , for now I have moved these to two functions

rht · 2023-10-30T04:00:31Z

mesa/experimental/jupyter_viz.py


-    space_ax.scatter(**portray(space))
+
+    # figure(figsize=(50, 50), dpi=80)


Needs to be removed.

introduced by automated merge, should be resolved

rht · 2023-10-30T04:01:42Z

mesa/experimental/jupyter_viz.py

+
+    # figure(figsize=(50, 50), dpi=80)
+    space_fig = Figure()
+    space_ax = space_fig.subplots()


This function already has space_ax in the argument, so these 2 lines are unnecessary.

Upon reading further, the whole new change from this line to L234 are duplicate of make_space. Please remove it.

introduced by automated merge, should be resolved

rht · 2023-10-30T04:59:44Z

Also take a look at your commits and see if you can group/squash them into the key changes. The goal is a easily readable where the key changes are easily identifiable.

My feedback with the current PR is that I have to review line by line comparing with the old version, because there could be unexpected changes being slipped in. This is more time consuming that necessary. And indeed there are: #1825 (comment).

rht · 2023-10-30T09:31:33Z

mesa/experimental/model_control.py

+                on_click=do_reset,
+            )
+
+            # with solara.Row(gap="10px", justify="center"):


This is another line that needs to be removed.

rht · 2023-10-30T09:37:01Z

I have found another line slipped in https://github.com/projectmesa/mesa/pull/1825/files#r1375915733, and having to review from scratch every line changed is taxing for me. Can you open a new PR that does the splitting of ModelController and UserInputs into separate files, and redo this PR cleanly over this atomic change once merged? There is enough time until CSSSA.

ankitk50 · 2023-10-30T09:46:26Z

I have found another line slipped in https://github.com/projectmesa/mesa/pull/1825/files#r1375915733, and having to review from scratch every line changed is taxing for me. Can you open a new PR that does the splitting of ModelController and UserInputs into separate files, and redo this PR cleanly over this atomic change once merged? There is enough time until CSSSA.

Sure

ankitk50 · 2023-10-31T22:55:39Z

The major changes related to the UI has been moved to #1854. I will create a separate PR for restructuring / reorganising.

ankitk50 added 3 commits September 25, 2023 18:03

caclulate agent size based on Number of Agents

452cca1

cap min size of agents to 5

b8e6c09

restructure the UI layout

59a5b99

ankitk50 mentioned this pull request Sep 26, 2023

Agent size change based on size of figure #1820

Closed

Corvince reviewed Sep 26, 2023

View reviewed changes

rht reviewed Oct 7, 2023

View reviewed changes

rht mentioned this pull request Oct 23, 2023

How to change the default UI of Mesa #1842

Closed

ankitk50 marked this pull request as ready for review October 29, 2023 20:36

rht reviewed Oct 30, 2023

View reviewed changes

mesa/experimental/jupyter_viz.py Outdated Show resolved Hide resolved

rht reviewed Oct 30, 2023

View reviewed changes

mesa/experimental/jupyter_viz.py Outdated Show resolved Hide resolved

rht reviewed Oct 30, 2023

View reviewed changes

Define UI layout for browser and jupyter

e8fd953

ankitk50 force-pushed the ui_layout branch from 8e8221c to e8fd953 Compare October 30, 2023 09:16

Merge branch 'main' into ui_layout

c464a33

rht reviewed Oct 30, 2023

View reviewed changes

tpike3 closed this Nov 10, 2023


		# render layout and plot

		if "ipykernel" in sys.argv[0]: # jupyter


		space_ax.scatter(**portray(space))

		# figure(figsize=(50, 50), dpi=80)

Uh oh!

Conversation

ankitk50 commented Sep 26, 2023

Uh oh!

Corvince commented Sep 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankitk50 commented Oct 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tpike3 commented Oct 30, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rht commented Oct 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rht commented Oct 30, 2023

Uh oh!

ankitk50 commented Oct 30, 2023

Uh oh!

ankitk50 commented Oct 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 30, 2023 •

edited

Loading