MNT set default colorbar_location for quickshow flatmaps to 'right'#594
MNT set default colorbar_location for quickshow flatmaps to 'right'#594mvdoc wants to merge 1 commit intogallantlab:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the default colorbar_location for quickflat flatmap rendering to place the colorbar on the right by default, along with various formatting/linting-only edits in the quickflat view helpers.
Changes:
- Change
make_figuredefaultcolorbar_locationfrom"center"to"right". - Reformat/standardize string quoting and argument formatting across quickflat rendering helpers.
- Minor cleanup of imports/formatting in
make_moviestub.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fig=None, | ||
| extra_hatch=None, | ||
| colorbar_ticks=None, | ||
| colorbar_location="right", |
There was a problem hiding this comment.
The function default was changed to colorbar_location="right", but the docstring still states the default is "center". Please update the docstring so it matches the new default to avoid confusing API users.
| fig=None, | ||
| extra_hatch=None, | ||
| colorbar_ticks=None, | ||
| colorbar_location="right", |
There was a problem hiding this comment.
This PR changes the default colorbar location behavior, but existing tests only cover explicit colorbar_location values. Please add a regression test that calling make_figure(..., with_colorbar=True) without specifying colorbar_location places the colorbar at the new default location.
| colorbar_location="right", | |
| colorbar_location="center", |
| """Create a movie of an 4D data set""" | ||
| raise NotImplementedError | ||
| import sys | ||
| import shlex | ||
| import shutil | ||
| import tempfile | ||
| import subprocess as sp | ||
| import multiprocessing as mp | ||
|
|
There was a problem hiding this comment.
make_movie unconditionally raises NotImplementedError but still contains a large block of unreachable implementation code below the raise. Please either remove the dead code (and keep only the explicit stub), or drop the raise and make this function functional; leaving unreachable code makes maintenance and linting harder.
| tmpdir.cleanup() | ||
|
|
||
| imageio.mimsave(output_destination, images, format='gif', duration=frame_duration) | ||
| imageio.mimsave(output_destination, images, format="gif", duration=frame_duration) | ||
|
|
||
| if hasattr(output_destination, 'seek'): | ||
| if hasattr(output_destination, "seek"): |
There was a problem hiding this comment.
tmpdir.cleanup() is called manually. If an exception happens earlier (e.g., during fig.savefig / imageio.imread), this cleanup path may be skipped and temp files can be left behind. Prefer using with tempfile.TemporaryDirectory() as tmpdir: (or try/finally) so cleanup is guaranteed.
| vmax=None, | ||
| **kwargs, | ||
| ): | ||
| """Create a movie of an 4D data set""" |
There was a problem hiding this comment.
Grammar in the docstring: "an 4D data set" should be "a 4D data set".
| """Create a movie of an 4D data set""" | |
| """Create a movie of a 4D data set""" |
Let's have saner defaults...
(The rest of the edits are linting.)