Skip to content

Add column / row wise normalization to imshow and pyrshow - #49

Merged
billbrod merged 17 commits into
LabForComputationalVision:mainfrom
ershook:row_col_vrange
Oct 22, 2026
Merged

billbrod merged 17 commits into
LabForComputationalVision:mainfrom
ershook:row_col_vrange

Conversation

@ershook

@ershook ershook commented May 1, 2026 •

Copy link
Copy Markdown
Contributor

This PR:

  • Adds column and row wise normalization to imshow and pyrshow.
  • Tests that figure titles match vmin/vmax of displayed images.
  • Tests that vmin/vmax of displayed images matches expected values.

(@billbrod EDIT to add summary):

  • Adds new vrange options for imshow/animshow/pyrshow:
    • "autoNcol" and "autoNrow", which is the same as "autoN" except values are computed across column / row respectively
    • "autoNcolcomplex", same as "autoNcol", except the two plots from complex values are considered together.
    • list of 2-tuples (same as number of images), where each vrange is set separately.
  • Adds additional "autoNpyr" for pyrshow, which is the same as "autoN" except values are computed across scales and the two residuals (if present) have their ranges determined independently (as in "indepN")
  • Fixes formatting for vrange docstring
  • Adds tests for vrange values
  • Reruns pyramid and steerable pyramid notebooks so the plots reflect these changes.

@billbrod billbrod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread TESTS/unitTests.py Outdated
Comment thread TESTS/unitTests.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated

@billbrod billbrod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.
  • 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_complex arg 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.

Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread TESTS/unitTests.py Outdated
Comment thread TESTS/unitTests.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated

@billbrod billbrod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 call plt.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)

Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread TESTS/unitTests.py
Comment thread TESTS/unitTests.py Outdated
@ershook

ershook commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

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 call plt.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)

For the notebooks the default behavior of pyrshow is to now use vrange='auto1pyr' so the residuals are being independently computed. I added a comment explaining this.

@billbrod billbrod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a bug, which we need to fix. Otherwise, my comments are just about some readability changes.

  • I found a bug with the residuals, which I've marked below. I've also marked the two changes that are necessary for the test to have caught it: the values for our fake residuals cannot be uniform, and the line to grab the expected clims was wrong (the same mistake as within the code).
  • I had messaged you something about how we should remove the notebook from this PR, but I forgot that we do need to rerun it, since the images have changed. Given that's the case, this is fine (though there is a new empty cell created at the end of the notebook that should be removed).

Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread TESTS/unitTests.py Outdated
Comment thread TESTS/unitTests.py Outdated
Comment thread TESTS/unitTests.py Outdated
Comment thread TESTS/unitTests.py Outdated
Comment thread TESTS/unitTests.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated
Comment thread src/pyrtools/tools/display.py Outdated

@billbrod billbrod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks for all your work!

@billbrod
billbrod merged commit 8a162c3 into LabForComputationalVision:main Oct 22, 2026
43 checks passed
@billbrod billbrod mentioned this pull request Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants