Feature: properly route multi-variable StateMonitors to subplots#64
Feature: properly route multi-variable StateMonitors to subplots#64Utkarsha2811 wants to merge 5 commits intobrian-team:masterfrom
Conversation
mstimberg
left a comment
There was a problem hiding this comment.
Thank you for the PR, I've left a few comments. Could you also add a test (only testing whether it works at all and returns the correct thing) and mention the feature in the documentation?
| try: | ||
| axes_arr = np.asarray(axes).ravel() | ||
| assert len(axes_arr) == n_vars | ||
| except Exception: | ||
| raise TypeError("If multiple variables are recorded, 'axes' must " | ||
| "be an iterable of matching length.") |
There was a problem hiding this comment.
Not to happy about the except Exception – maybe just check if len(axes_arr) == n_vars? If something fails during the conversion to an array, this itself should already raise a meaningful error, I think.
| if 'var_name' not in kwds_var: | ||
| kwds_var['var_name'] = var_name | ||
| if 'var_unit' not in kwds_var and isinstance(values, Quantity): | ||
| kwds_var['var_unit'] = _get_best_unit(values) |
There was a problem hiding this comment.
I wonder whether we wouldn't need to allow for a slightly more complex syntax here, since a single var_name or var_unit argument doesn't make much sense when a user recorded several variables. Maybe a mapping? Something along the lines of:
mon = StateMonitor(group, ["I", "v"], record=True)
# ...
brian_plot(mon,
var_name={"I": "input current",
"v": "membrane potential"},
var_unit={"I": nA, "v", mV})| n_vars = len(monitor.record_variables) | ||
| if n_vars == 1: | ||
| var_name = monitor.record_variables[0] | ||
| values = getattr(brian_obj, var_name).T | ||
| if 'var_name' not in kwds: | ||
| kwds['var_name'] = var_name | ||
| if 'var_unit' not in kwds and isinstance(values, Quantity): | ||
| kwds['var_unit'] = _get_best_unit(values) | ||
| return plot_state(brian_obj.t, values, axes=axes, **kwds) | ||
| else: |
There was a problem hiding this comment.
This part is not quite correct: if you hand over a StateMonitorView, you only have a single variable. The situation here is code like:
mon = StateMonitor(group, ["I", "v"], record=True)
brian_plot(mon.I) # mon.i is a StateMonitorViewThe check verifies whether the monitor recorded several variables, but this doesn't matter here.
…sts & docsAddress review comments: extract helpers, support dict kwargs, add tests & docs
|
Test results (all passing): brian2tools/tests/test_plotting.py::test_plot_monitors PASSED [ 20%] |
|
Thanks for the changes, I won't be able to review this right away – please be patient 🙏 |
Hey @mstimberg, I’ve rebased the PR on top of master. Could you take a look when you get a chance? Tried to address issues mentioned previously. |
|
Hi @Utkarsha2811 Sorry for the very late reply, I was down with a cold recently and am very much behind with PR reviews… I'll hopefully find some time to review/test in detail tomorrow, but one thing I already saw: could you please include an image demonstrating this new feature in the documentation? |
mstimberg
left a comment
There was a problem hiding this comment.
Hi @Utkarsha2811, this looks all very good to me now. I made a few nitpicking comments below, but those are not essential. The only important missing thing is a picture in the documentation IMO.
| raise TypeError( | ||
| "If multiple variables are recorded, 'axes' must be an " | ||
| "array-like of Axes with length %d (got %d)." | ||
| % (n_vars, len(axes_arr))) |
There was a problem hiding this comment.
Very minor detail, but I'd prefer an f-string over the old-school formatting used here.
| % (n_vars, len(axes_arr))) | ||
|
|
||
| ret_axes = [] | ||
| for i, var_name in enumerate(record_variables): |
There was a problem hiding this comment.
Again a nitpicking detail, but I'd prefer the more Pythonic
for ax, var_name in zip(axes_arr, record_variables):| plt.close() | ||
|
|
||
| # StateMonitorView of a multi-variable monitor | ||
| axes = brian_plot(state_mon[3]) |
There was a problem hiding this comment.
I actually realized that an earlier comment of mine was incorrect – I thought e.g. state_mon.v would return a StateMonitorView (in the same way that for a NeuronGroup group, the access group.v would return a VariableView), but you are right that in StateMonitor, a "view" is used when you index into the group. I wonder whether it would make sense to change something on the Brian side so that you could call e.g. brian_plot(state_mon.v) (which currently does not work, since state_mon.v is a simple Quantity without information about the variable name). But this is outside the scope of this PR, of course.
|
Changes made as requested, @mstimberg. Thanks! |
Closes #8
Summary
Changes