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

Binder -- Enable online viewing/running of Jupyter examples #107

Merged
merged 15 commits into from
Mar 19, 2021
Merged

Conversation

lyndond
Copy link
Collaborator

@lyndond lyndond commented Mar 6, 2021

Changes:

  • Fixed bug w/ eigendistortions when k=1
  • from tqdm.auto import tqdm for nicer notebook formatting when running synthesis with eigendistortions
  • Re-ran 02_Eigendistortions.ipynb and Demo_Eigendistortion.ipynb w/ fixed code
  • removed pt.imshow from notebooks
  • Added Binder badges to README.md that will always point to master branch examples folder
  • Added Binder badges to individual ipynb for 02_Eigendistortions.ipynb and Demo_Eigendistortion.ipynb so people will be able to open them online from readthedocs.io or from the github repo
  • Added packages and subpackages to setup.py in order for Binder to pip install properly. Otherwise, it just throws package not found errors.
  • Limit setup.py to torch<1.8 because steerpyr is breaking tons of steerpyr related tests bc of the torch.fft module, but Nikhil's future PR should fix it.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

setup.py Show resolved Hide resolved
Copy link
Collaborator Author

@lyndond lyndond left a comment

Choose a reason for hiding this comment

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

added simple in-line comments explaining changes

@codecov-io
Copy link

codecov-io commented Mar 6, 2021

Codecov Report

Merging #107 (92590fb) into master (a3d031d) will decrease coverage by 6.35%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
- Coverage   76.63%   70.28%   -6.36%     
==========================================
  Files          36       36              
  Lines        3463     3463              
  Branches      741      741              
==========================================
- Hits         2654     2434     -220     
- Misses        618      843     +225     
+ Partials      191      186       -5     
Impacted Files Coverage Δ
plenoptic/synthesize/eigendistortion.py 17.68% <25.00%> (-71.43%) ⬇️
plenoptic/metric/model_metric.py 28.57% <0.00%> (-71.43%) ⬇️
plenoptic/synthesize/autodiff.py 20.00% <0.00%> (-66.67%) ⬇️
...simulate/canonical_computations/non_linearities.py 20.51% <0.00%> (-64.11%) ⬇️
plenoptic/metric/perceptual_distance.py 67.92% <0.00%> (-17.93%) ⬇️
plenoptic/tools/signal.py 33.72% <0.00%> (-15.70%) ⬇️
plenoptic/simulate/models/frontend.py 89.79% <0.00%> (-10.21%) ⬇️
plenoptic/tools/data.py 70.76% <0.00%> (-9.24%) ⬇️
plenoptic/simulate/models/ventral_stream.py 78.54% <0.00%> (-0.70%) ⬇️

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 a3d031d...92590fb. Read the comment docs.

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.

The only change I think we need is the author/author_email fields of setup.py, but there are a couple of places I'd like to start a conversation (and probably make issues for)

plenoptic/synthesize/eigendistortion.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@billbrod
Copy link
Collaborator

billbrod commented Mar 8, 2021

Looking at the notebooks, it looks like the synthesis didn't actually run? (all the progress bars say 0/X) But the images are there, so I'm confused.

Copy link
Collaborator Author

lyndond commented Mar 10, 2021

Yeah I see that too... strange. Maybe it's tqdm.notebook bugging out. Can't have our cake and eat it too?

@billbrod
Copy link
Collaborator

Ah yeah, that could definitely be it. Very strange.

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.

I'm not sure that util import is necessary, but otherwise I think this is good to go.

setup.py Show resolved Hide resolved
@billbrod
Copy link
Collaborator

Oh one other requested change: can you document somewhere the steps to get tqdm working in notebooks? with tqdm.auto, it won't fallback to the ascii version if it can't use the fancy one -- it will raise confusing exceptions (speaking from experience). So our requirements should point out that, in addition to installing jupyter, they'll also need ipywidgets (I think that's the only extra requirement)

billbrod and others added 2 commits March 18, 2021 13:57
This will eventually get moved to a separate document, but so we have it
for now
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.

All looks good to me!

README.md Show resolved Hide resolved
@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

ThomasYerxa commented on 2021-03-18T19:11:01Z
----------------------------------------------------------------

could be just me but I couldn't see the most noticeable distortion at first, I guess that is the whole point but may want to consider cranking up the distortion factor?


@ThomasYerxa
Copy link
Contributor

looks good to me, left one comment on the tutorial notebook that isn't related to this PR.

@lyndond lyndond merged commit 2405bac into master Mar 19, 2021
@lyndond lyndond deleted the binder branch March 19, 2021 11:48
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