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.
billbrod
left a comment
There was a problem hiding this comment.
We're almost there!
- End of the steerable pyramid notebook says "note need additional argument pyr.is_complex to pyrshow because default is False", which should be removed. Also add a line here about setting
show_residuals=False. - In notebook section 3.2.2, we plot pyrshow with
show_residuals=True. add a brief comment explaining why the residual highpass looks like a solid black imgae. - animshow docstring about vrange has dropped the first paragraph about 2-tuples and lists thereof, can you add that back?
- colormap_range docstring about vrange is formatted incorrectly (indents/newlines are wrong) -- can you fix it to match the others?
- actually, looks like the text for colormap_range vrange descriptions also hasn't been updated to match your last commit, can you update them as well?
- Can you add
plt.close("all")to the end of each of your vrange tests? matplotlib starts complaining because we have a lot of figures open (alternatively, the better way to do this is probably juts to callplt.close(fig)on the specific figure, but that would require changing the tests a bit to make sure that variable is captured. up to you)
| raise ValueError("indep and col cannot be used together in vrange. Use either indep or " \ | ||
| "auto[x]col or auto[x]colcomplex") |
There was a problem hiding this comment.
| raise ValueError("indep and col cannot be used together in vrange. Use either indep or " \ | |
| "auto[x]col or auto[x]colcomplex") | |
| raise ValueError("indep and col cannot be used together in vrange. Use either indep or " | |
| "auto[x]col or auto[x]colcomplex") |
I think that's the right indenting. Just avoid the slash.
| of 2-tuples, each image has an independent vmin/vmax, where each images | ||
| minimum/maximum values are specified by the 2-tuples in the list ordered | ||
| from first image to last. If a string: |
There was a problem hiding this comment.
| of 2-tuples, each image has an independent vmin/vmax, where each images | |
| minimum/maximum values are specified by the 2-tuples in the list ordered | |
| from first image to last. If a string: | |
| of 2-tuples, must have one per image, and each image's vmin/vmax is specified | |
| by the corresponding 2-tuple. If a string: |
There was a problem hiding this comment.
and the same for the animshow, pyrshow, colormap_range
| method described above. | ||
|
|
||
| * ``'autoNcolcomplex'``: where ``N`` is an integer. Same as ``'autoNcol'``, | ||
| but the vmin/vmax are computed across the real and imaginary parts in each column. |
There was a problem hiding this comment.
| but the vmin/vmax are computed across the real and imaginary parts in each column. | |
| but the vmin/vmax are computed jointly across the real and imaginary parts in each column. |
and same for the other functions
|
|
||
| .. attention:: | ||
| this only affects the behavior for grayscale images. RGB images | ||
| will always be displayed with vrange [0, 1] (for floats) or [0, 255] | ||
| (for ints), because of how matplotlib handles them. | ||
|
|
There was a problem hiding this comment.
| .. attention:: | |
| this only affects the behavior for grayscale images. RGB images | |
| will always be displayed with vrange [0, 1] (for floats) or [0, 255] | |
| (for ints), because of how matplotlib handles them. |
pyramids only accept 1d and 2d inputs, so they can't actually run them on RGB imgaes
| for mode in ['auto', 'indep', 'autoNrow', 'autoNcol','autoNcolcomplex']: | ||
| for mode_num in range(4): | ||
| for img_idx in range(4): | ||
| with self.subTest(mode=mode, img_idx=img_idx): | ||
| if 'N' in mode: | ||
| mode_full = f"{mode.replace('N',str(mode_num))}" | ||
| else: | ||
| mode_full = f"{mode}{mode_num}" | ||
| vmin, vmax = self._get_clims(self._imshow(mode_full))[img_idx] | ||
| exp_vmin, exp_vmax = self._expected_clims(mode_full)[img_idx] | ||
| self.assertTrue(np.isclose(vmax, exp_vmax, atol=1e-6)) | ||
| self.assertTrue(np.isclose(vmin, exp_vmin, atol=1e-6)) |
There was a problem hiding this comment.
| for mode in ['auto', 'indep', 'autoNrow', 'autoNcol','autoNcolcomplex']: | |
| for mode_num in range(4): | |
| for img_idx in range(4): | |
| with self.subTest(mode=mode, img_idx=img_idx): | |
| if 'N' in mode: | |
| mode_full = f"{mode.replace('N',str(mode_num))}" | |
| else: | |
| mode_full = f"{mode}{mode_num}" | |
| vmin, vmax = self._get_clims(self._imshow(mode_full))[img_idx] | |
| exp_vmin, exp_vmax = self._expected_clims(mode_full)[img_idx] | |
| self.assertTrue(np.isclose(vmax, exp_vmax, atol=1e-6)) | |
| self.assertTrue(np.isclose(vmin, exp_vmin, atol=1e-6)) | |
| for mode in ['auto{}', 'indep{}', 'auto{}row', 'auto{}col','auto{}colcomplex']: | |
| for mode_num in range(4): | |
| mode_full = mode.format(mode_num) | |
| for img_idx in range(4): | |
| with self.subTest(mode_full=mode_full, img_idx=img_idx): | |
| vmin, vmax = self._get_clims(self._imshow(mode_full))[img_idx] | |
| exp_vmin, exp_vmax = self._expected_clims(mode_full)[img_idx] | |
| self.assertTrue(np.isclose(vmax, exp_vmax, atol=1e-6)) | |
| self.assertTrue(np.isclose(vmin, exp_vmin, atol=1e-6)) | |
| plt.close('all') | |
cleans that up a little
| vrange=vrange, cmap=None, n_cols=2) | ||
| return clims | ||
|
|
||
| def test_vrange_max_vmin_matches_expected(self): |
There was a problem hiding this comment.
| def test_vrange_max_vmin_matches_expected(self): | |
| def test_vrange_vmax_vmin_matches_expected(self): |
right?
There was a problem hiding this comment.
or test_vrange_matches_expected is probably just as good
This PR: