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

Steer pyr demo #119

Merged
merged 56 commits into from
May 17, 2023
Merged

Steer pyr demo #119

merged 56 commits into from
May 17, 2023

Conversation

ThomasYerxa
Copy link
Contributor

Demonstrates the use of steer pyr by (1) using it as a front end for a conv net classifier on FashionMNIST and (2) comparing the eigendistortions w/ a baseline classifier that doesn't use a steer pyr frontend.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ThomasYerxa ThomasYerxa marked this pull request as draft March 29, 2021 16:51
@codecov-io
Copy link

codecov-io commented Mar 29, 2021

Codecov Report

Merging #119 (23ae52d) into master (37758e1) will decrease coverage by 47.42%.
The diff coverage is 12.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #119       +/-   ##
===========================================
- Coverage   75.42%   27.99%   -47.43%     
===========================================
  Files          33       33               
  Lines        2629     2568       -61     
  Branches      526      516       -10     
===========================================
- Hits         1983      719     -1264     
- Misses        505     1777     +1272     
+ Partials      141       72       -69     
Impacted Files Coverage Δ
plenoptic/tools/signal.py 16.66% <ø> (-34.07%) ⬇️
...e/canonical_computations/steerable_pyramid_freq.py 6.64% <6.77%> (-82.96%) ⬇️
plenoptic/tools/display.py 10.23% <50.00%> (-56.31%) ⬇️
plenoptic/tools/data.py 51.58% <100.00%> (-21.49%) ⬇️
plenoptic/synthesize/mad_competition.py 9.97% <0.00%> (-75.88%) ⬇️
plenoptic/metric/model_metric.py 28.57% <0.00%> (-71.43%) ⬇️
plenoptic/metric/perceptual_distance.py 15.09% <0.00%> (-70.76%) ⬇️
...simulate/canonical_computations/non_linearities.py 20.51% <0.00%> (-64.11%) ⬇️
plenoptic/synthesize/synthesis.py 22.10% <0.00%> (-51.88%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37758e1...23ae52d. Read the comment docs.

@ThomasYerxa
Copy link
Contributor Author

converted to PR prematurely. closing for now

@ThomasYerxa
Copy link
Contributor Author

When the dust settles, the only significant change coming from this branch should be the Demo_Steerable_Pyramid.ipynb notebook. That should be relatively stable to the one appearing in this PR, so feel free to glance at the notebook if curious but ignore everything else.

@ThomasYerxa ThomasYerxa reopened this Mar 29, 2021
@lyndond lyndond requested review from nikparth and lyndond March 29, 2021 20:29
@ThomasYerxa
Copy link
Contributor Author

Questions:

  1. Was there a particular reason we opted for a frequency space implementation (boundary conditions are nicer etc.)? In "1.1: Visualizing wavelets" it would be nice to mention if so.
  2. For the steerability demo: is this sufficient/interesting? would it be more compelling i.e. to steer to an intermediate orientation and then show that if you initialize a pyramid that includes the steered-to-orientation you can recover the same coefficients.

@billbrod
Copy link
Collaborator

billbrod commented Apr 21, 2023

I've gone through and edited the text here, take another look at it and tell me what you think. @nikparth can you also take a look at this?

  • I think the final section, on probing the effect of using the steerable pyramid as a frontend, is a great idea, but I think we need some more text at the end to explain. I tried adding some, but it still feels like we ran out of steam while writing it and are just saying "whelp, you make sense of it" . I also think need a bit more detail about what the networks are being trained to do, some framing, etc.
  • I'm tempted to actually split that whole section off into a separate notebook, so we have two notebooks: steerable pyramid basics and steerable pyramid-as-model component. I could add something about classic visual models there too. Thoughts?
  • Re: your question 1. I think it's because that was the only one we really want people to use: it handles any order, reconstruction is exact, and allows for the complex-valued output (which is really useful). @nikparth do you remember if there was another reason?
  • Re: question 2. I think this is fine for now, but agree that steering an intermediate representation would be more compelling. Can you add info to Add demo showing steering of intermediate representation #204 ? Add all the notes you have, any code, etc, so that you or someone else can pick it up later.

I also pushed some small updates to (unrelated) docstrings, because I noticed them and they were annoying me.

And here it is on the readthedocs

Copy link
Collaborator

@nikparth nikparth left a comment

Choose a reason for hiding this comment

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

I think overall this seems fine as a first release. I added some comments on wording in the notebook, but my main comments would be to remove the standard CNN demonstration and comparison to the pyramid. I think it's fine to just show that the pyramid can be added as a module to build on with standard pytorch modules. And then I'd say remove the eigendistortion section as well and we could have another notebook that is about using plenoptic methods with canonical visual models.

@billbrod billbrod self-requested a review as a code owner May 12, 2023 16:08
@billbrod billbrod merged commit 77a2670 into main May 17, 2023
@billbrod billbrod deleted the steer_pyr_demo branch May 17, 2023 15:29
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.

5 participants