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

Make Googlnet & InceptionNet scriptable #1349

Merged
merged 12 commits into from
Sep 27, 2019

Conversation

eellison
Copy link
Contributor

@eellison eellison commented Sep 18, 2019

Based on the conclusion from #1273: we are always returning a tuple when scripting.

This was blocked by pytorch/pytorch#26683.

@eellison
Copy link
Contributor Author

blocked by pytorch/pytorch#26437

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

LGTM!

@fmassa fmassa closed this Sep 20, 2019
@fmassa fmassa reopened this Sep 20, 2019
@eellison eellison changed the title [WIP] [No Review] make googlnet scriptable Make Googlnet & InceptionNet scriptable Sep 24, 2019
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

LGTM, except for the test, which has a spurious assert now

@@ -55,7 +53,7 @@ def check_script(self, model, name):
tb = traceback.format_exc()
scriptable = False
msg = str(e) + str(tb)
self.assertEqual(torchub_models[name], scriptable, msg)
self.assertEqual(scriptable, scriptable, msg)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this change is what you want to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since they are all scriptable now, we don't need to access torchub_models and just assert that scriptable is True

Copy link
Member

Choose a reason for hiding this comment

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

Can you then make

self.assertEqual(scriptable, True, msg)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh oops - good catch

@fmassa
Copy link
Member

fmassa commented Sep 24, 2019

Test failures look legit

@eellison
Copy link
Contributor Author

@pytorchbot rebase this please

@codecov-io
Copy link

codecov-io commented Sep 26, 2019

Codecov Report

Merging #1349 into master will increase coverage by 0.03%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1349      +/-   ##
==========================================
+ Coverage    63.9%   63.94%   +0.03%     
==========================================
  Files          78       78              
  Lines        6147     6176      +29     
  Branches      940      944       +4     
==========================================
+ Hits         3928     3949      +21     
- Misses       1941     1947       +6     
- Partials      278      280       +2
Impacted Files Coverage Δ
torchvision/models/inception.py 85.35% <64.7%> (-0.93%) ⬇️
torchvision/models/googlenet.py 73.54% <71.42%> (+0.16%) ⬆️

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 cae0eee...1968763. Read the comment docs.

@eellison eellison merged commit b9cbc22 into pytorch:master Sep 27, 2019
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.

3 participants