Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PortillaSimoncelliMinimal to tutorial get original paper's statistics #216

Merged
merged 10 commits into from
Dec 11, 2023

Conversation

dherrera1911
Copy link
Collaborator

Add a new section to Portilla Simoncelli metamers tutorial, with a demonstration of how to recover the original set of statistics reported in the paper from the output of the model. closes #151

@dherrera1911 dherrera1911 requested a review from billbrod as a code owner August 3, 2023 21:14
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8573cb2) 86.82% compared to head (664da25) 86.79%.

❗ Current head 664da25 differs from pull request most recent head 0f8f094. Consider uploading reports for the commit 0f8f094 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #216      +/-   ##
==========================================
- Coverage   86.82%   86.79%   -0.03%     
==========================================
  Files          35       35              
  Lines        3346     3347       +1     
==========================================
  Hits         2905     2905              
- Misses        441      442       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@review-notebook-app
Copy link

review-notebook-app bot commented Aug 3, 2023

View / edit / reply to this conversation on ReviewNB

dherrera1911 commented on 2023-08-03T21:28:40Z
----------------------------------------------------------------

Cut line


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 3, 2023

View / edit / reply to this conversation on ReviewNB

dherrera1911 commented on 2023-08-03T21:28:41Z
----------------------------------------------------------------

Maybe should be 'dropped from full set'


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 3, 2023

View / edit / reply to this conversation on ReviewNB

dherrera1911 commented on 2023-08-03T21:28:42Z
----------------------------------------------------------------

Typo, text -> test


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 3, 2023

View / edit / reply to this conversation on ReviewNB

dherrera1911 commented on 2023-08-03T21:28:43Z
----------------------------------------------------------------

This 2-level list rendered better in jupyter lab.


PS and steer pyr were inconsistent in the default arg that means "full
representation": steer pyr used an empty list, while PS used None.
Switch steer pyr to use None, updates docstrings rto reflect this

Also makes PS remove_scales method public, because it might be helpful
for people. and then fleshed out its docstring and removed the device
arg, because it's redundant
Copy link
Collaborator

@billbrod billbrod left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks for putting this together! I've made some small changes to the following:

  • change to index tensor type to make it run on python 3.7
  • use coarse-to-fine in final synthesis example, update model_min's forward pass to support that
  • small wording changes, line wraps, spacing, capitalization convention (camel case -> snake case)

Outside of the changes you made, I also made some we had discussed earlier:

  • in PS docstring, clarifies that, when use_true_correlations=True, the diagonals are still un rescaled, so that they're covariances)
  • Adds a brief discussion of how to get proper coarse-to-fine integration when extending the PS model (to the notebook)
  • makes the steerable pyramid's default / "all" argument for scales match that of the PS model (None, rather than [])

There's only one thing:

  • Can we find something to link to about the auto-covariance matrix symmetry? And maybe why it's difference from the regularly symmetry of covariance matrices.

@eerosim
Copy link
Contributor

eerosim commented Sep 21, 2023

Daniel,
Thanks so much for putting the time into tracking down the redundancies in the parameter file! A few notes:

  • In the list at the top, I think it's worth providing the math expressions for the number of non-redundant correlational parameters (auto-correlation, cross-correlation, etc). You currently provide those later, when tallying up the numbers, but I think it's preferable to state them up front when you describe the issue.
  • The magMeans are different (for reasons I cannot remember, they are in the code but do not seem to do much). Unlike the correlations, they they are only approximately redundant, and I'm wondering if we can make the case for their elimination more precise. Specifically, in your comparison of textures generated by the full and minimal model:
    1. I believe you're running your examples from the same starting image. Can you also use identical seeds for the random number generator (to guarantee that the stochastic Adam optimizer draws the same random values)? I think that should yield images that are much more similar, perhaps even visually indistinguishable.
    2. Report the RMSE between the two synthesized images.
    3. Compare the values of the magMeans in the two results. I think they should be nearly the same, even though they are not explicitly "enforced" in the minimal model (and if so, this demonstrates that they are statistically redundant).

Thanks much,
Eero

@billbrod
Copy link
Collaborator

There are some more changes we'd like to see to the text at the bottom of the notebook, but given the significant changes to the PS code in #225 , I'm going to merge this PR and we'll finish the changes over in the new PR. I'll copy over Eero's comments.

@billbrod billbrod merged commit 99d4f03 into main Dec 11, 2023
74 of 75 checks passed
@billbrod billbrod deleted the psMinStats branch December 11, 2023 17:12
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.

Portilla Simoncelli Model - Number of statistics don't match paper
3 participants