Add an option to disable drawing agent space #1778

[rlskoeser]

as discussed on #1778

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.83% 🎉

Comparison is base (494c917) 79.88% compared to head (f56ee0f) 81.72%.

❗ Current head f56ee0f differs from pull request most recent head ae76fdd. Consider uploading reports for the commit ae76fdd to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1783      +/-   ##
==========================================
+ Coverage   79.88%   81.72%   +1.83%     
==========================================
  Files          15       15              
  Lines         880      881       +1     
  Branches      188      189       +1     
==========================================
+ Hits          703      720      +17     
+ Misses        154      136      -18     
- Partials       23       25       +2     

Files Changed	Coverage Δ
mesa/experimental/jupyter_viz.py	31.11% <100.00%> (+12.45%)	⬆️

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

[rht]

I have seen None and False being used as distinct possible values in a parameter in a library somewhere. Can't remember which one. But it's easy to confuse the 2, because None is considered falsy. I am not fully sure yet of the best way forward in this situation.

[rht]

Maybe a string "disabled"? @Corvince thoughts?

[tpike3]

Thanks @rlskoeser!

This change is making me wonder if we should switch to a different naming convention as the False, None, Else took me a while to think through, which I worry will confuse users.

Maybe kwargs for different space and not space options?

Thoughts @rlskoeser?
Thoughts @rht?
Thoughts @ankitk50

[ankitk50]

'None' should represent absence of a value which seems to be the case here.

[Corvince]

I think space_drawer=None reads pretty good and should be unambiguous.

That said I think we shouldn't assume what and how people want to plot there models.

I think it would be easier to let users just decide on which solara components they want to display (and provide some components)

[rht]

I think it would be easier to let users just decide on which solara components they want to display (and provide some components)

The reason why I set the space drawing to be automatic was to cover the 80% of the use case to be easy. The previous Tornado version does require users to specify manually, but I find this too much boilerplate code.

[rlskoeser]

I was thinking of None as the default / unspecified option, but it's plausible that someone would specify specify_drawer=None and expect to have no space and I agree that the code isn't as readable.

I was wondering if there was a different way to pass in the default make_space method instead of using None to flag the default, but when I tried that locally I couldn't pass through the False / None value; maybe because there's something I'm missing about how the JupyterViz method generates a MesaComponent.

I thought about a defined constant for either the default or the disabled state, but adding an optional kwarg parameter to disable drawing the space as @tpike3 suggested sounds easier to use and more readable to me. Although I'm not sure of a brief, readable name for that argument.

[rht]

What about passing a do-nothing lambda: None function and document this in the docstring?

[rht]

As in, if user doesn't want the space to be drawn, they should pass in the empty lambda function.

[rht]

Another option is to set space_drawer=make_space as the default argument, and if user specifies it with a falsy value (None, False, 0, etc), then don't draw anything.

[rlskoeser]

Another option is to set space_drawer=make_space as the default argument, and if user specifies it with a falsy value (None, False, 0, etc), then don't draw anything.

I tried this and it didn't work (the None turned into default behavior somewhere - I think related to the component creation). I will try again with the refactored jupyterviz code and see if I can get it to work. This seems like the cleanest option to me.

[rht]

You could also do space_drawer="default" and then replace is with make_space, and do nothing for falsy values.

[tpike3]

I don't want to stand in the way of merging this as it is an unequivocally an improvement;

The doc build issue was fixed with #1785

But as I opened a can of worms, any issue merging @rht, @Corvince, @jackiekazil ?

[rht]

But it's easy to confuse the 2, because None is considered falsy.

I stand by the statement of the problem of using None and False in a different way, so this PR can't be merged yet.

[tpike3]

You could also do space_drawer="default" and then replace is with make_space, and do nothing for falsy values.

I am good with this solution

[Corvince]

You could also do space_drawer="default" and then replace is with make_space, and do nothing for falsy values.

I am good with this solution

me too

[tpike3]

@rlskoeser This is your PR, so you are lead, let us know your thoughts or if there is any support we can provide.

[jackiekazil]

You could also do space_drawer="default" and then replace is with make_space, and do nothing for falsy values.

I am good with this solution

me too

+1

[rlskoeser]

Thanks for the input, everyone. I've re-implemented based on the latest refactor of the jupyter viz code with the proposed 'default' option, which does seem to be the simplest solution. I added a docstring for JupyterViz (although not sure my description of the measures parameter is accurate/helpful) and a minimal unit test that confirms the expected behavior for various options for drawing the space (default, custom, none).

[rht]

The ruff lint failed.

[tpike3]

I think this is a super simple fix ... https://beta.ruff.rs/docs/rules/unsorted-imports/ and the only issue I see

[tpike3]

@rlskoeser This is great and thanks for the tests! I think 2 commits is good, one for the improvement and one for the tests, but please squash the lint correction

Again thank you very much!

[rht]

It's fine to not squash the commits. We can do "squash and merge" later instead.

[rlskoeser]

Sorry I missed the linting error! I guess I've gotten too used to the isort pre-commit hook that I've started using on other projects.

Corrected now, and I amended & squashed the commit history.

[jackiekazil]

I am excited about this addition. Thank you for your contribution. <3
I haven't had a chance to test, so I will defer to someone else to make the final review/merge.