Conversation
billbrod
left a comment
There was a problem hiding this comment.
Thanks for putting this together Erica! I don't have time to do a full review right now, so I've added some small comments and will get back to this in more detail tomorrow.
billbrod
left a comment
There was a problem hiding this comment.
- in the vrange docstrings, all the single backticks should be double (I've marked this in one of them).
- this is an existing problem, where single backticks are italics for rST (which is what sphinx uses), but monospace for markdown (e.g.,
text), and we just accidentally used the wrong formatting here.
- this is an existing problem, where single backticks are italics for rST (which is what sphinx uses), but monospace for markdown (e.g.,
- I marked my suggestion for the explanation of the new options in one place, but it should be the same everywhere except pyrshow (which is the only other one I've added a comment to).
- The global/row/col tests can all be combined by iterating through strings and using subtest instead of just the ints (because the corresponding tests are otherwise identical), e.g.,:
def test_global_vrange_title_matches_clim(self):
modes = ["indep{}", "auto{}", "auto{}row", "auto{}col"]
modes = [m.format(i) for i in range(4) for m in modes]
for mode in modes:
for img_idx in range(4):
with self.subTest(mode=mode, img_idx=img_idx):
fig = self._imshow(mode")
clim_vmin, clim_vmax = self._get_clims(fig)[img_idx]
title_vmin, title_vmax = self._get_title_clims(fig)[img_idx]
self.assertEqual("{:.1e}".format(clim_vmin), "{:.1e}".format(title_vmin))
self.assertEqual("{:.1e}".format(clim_vmax), "{:.1e}".format(title_vmax))I think the only test where the logic is different for the different modes is share_clim. Those I think can all be separate, because it's more work than its worth to make them into a single test. But an additional one should be added for the indep vranges to make sure their clims are all different!
- pyrshow has an
is_complexarg that we should remove since we can infer whether the input is complex or not pretty easily and all it does is double col_wrap.
| self.assertTrue(np.isclose(vmin, exp_vmin, atol=1e-6)) | ||
|
|
||
| def test_global_vrange_vmax(self): |
There was a problem hiding this comment.
I would combine these into one test
| exp_vmin, _ = self._expected_clims(f"auto{mode}row")[img_idx] | ||
| self.assertTrue(np.isclose(vmin, exp_vmin, atol=1e-6)) | ||
|
|
||
| def test_row_vrange_vmax(self): |
| vrange_tmp, cmap = colormap_range(imgs_formatted, contains_rgb, vrange=vrange.split('complex')[0], n_cols=n_cols//2) | ||
| vrange_tmp = [v for v in vrange_tmp for _ in range(2)] | ||
| else: | ||
| vrange_tmp = [None] * len(image) |
There was a problem hiding this comment.
why is this here? why not just this be vrange_tmp = [] and then use vrange_tmp.append(vr[k]) below?
This PR: