-
-
Notifications
You must be signed in to change notification settings - Fork 431
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
Added model zoo #232
Added model zoo #232
Conversation
setup.py
Outdated
@@ -21,6 +21,7 @@ | |||
'numpy', | |||
'scipy', | |||
'setuptools', | |||
'pytest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete
requirements-dev.txt
Outdated
@@ -9,3 +9,6 @@ flake8 >= 3.3.0 | |||
python-coveralls >= 2.9.1 | |||
pillow >= 4.1.1 | |||
randomgen >= 1.14.4 | |||
requests >= 2.18.4 | |||
GitPython >= 2.1.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add to setup.py not to -dev.txt
foolbox/zoo/weights_fetcher.py
Outdated
|
||
|
||
def _filename_from_uri(url): | ||
filename = url.rsplit('/', 1)[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use split instead of rsplit
@@ -9,3 +9,6 @@ flake8 >= 3.3.0 | |||
python-coveralls >= 2.9.1 | |||
pillow >= 4.1.1 | |||
randomgen >= 1.14.4 | |||
requests >= 2.18.4 | |||
GitPython >= 2.1.10 | |||
responses >= 0.9.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add to -dev.txt
Great job! Once the final changes that we just discussed have been integrated this can be merged. Please don't forget to add comments for the less obvious parts. |
Looks good! Two comments regarding the example in the docs:
|
From a user's point of view the API is basically this:
Model owners wanting to make the model foolbox-ready need to put a
foolbox_model.py
containing a simplecreate()
function into the root folder of their git repository.Besides https://github.com/bethgelab/analysis-by-synthesis-model, I have done this already for three other models:
Model providers who provide the weights of their models in another place than the git repo itself, get use the following foolbox.zoo utilities to download the weights:
Full example: https://github.com/bethgelab/cifar10_challenge/blob/master/foolbox_model.py#L10
This test here: https://github.com/bveliqi/foolbox/blob/6a54bfafc89723fb9c73293ba7593a93b26a0dfe/foolbox/tests/test_model_zoo.py is more of an integration test than a unit test and depends on the git/weights providers being up & accessible. So I'm not sure whether we want to run these each & every time on our CI systems since they might take a bite more time compared to our other tests until now...
Also, I will have to add the documentation for the model zoo - but you can already review the code while I'll do that.