-
Notifications
You must be signed in to change notification settings - Fork 29
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
Keras interface and Python 3 compatability #3
Conversation
Compatability imports Print parens Fix iters Fix imports
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.
LGTM just see a few comments
README.md
Outdated
## NEW: an easy-to-use Keras interface | ||
|
||
Just in time for NIPS 2017, we're releasing an easy-to-use | ||
[Keras](https://keras.io/) interface for TANDA. Just train your TAN, load the |
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.
Re: "Just train your TAN...": I'd say let's lead with a one line (bolded and red?) about Keras interface + pre-trained TAN. Then mention can be used with any TAN you train / is implied
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.
I'd suggest something like: "Just in time for NIPS 2017, we're releasing an easy-to-use, pre-trained substitute for Keras' ImageDataGenerator data augmentation class. Just swap in TANDAImageDataGenerator
and you'll be using our trained data augmentation generators!"
@@ -1,6 +1,6 @@ | |||
#!/bin/bash | |||
source set_env.sh | |||
python experiments/cifar10/train.py \ | |||
python -m experiments.cifar10.train \ |
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.
fancy
experiments/cifar10/dataset.py
Outdated
|
||
def load_cifar10_batch(fpath, one_hot=True, as_float=True): | ||
with open(fpath, 'rb') as f: | ||
data = cPickle.load(f) | ||
data = cPickle.load(f, encoding='latin1') |
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.
?
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.
Many stacks overflowed here. Just let it be.
self.tan = tan | ||
self.session = K.get_session() | ||
|
||
def random_transform(self, x, seed=None): |
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.
Minor: Update doc string just to clarify that this is not in fact a random transform anymore, just maintaining same interface?
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.
On it
""" | ||
|
||
def __init__(self, | ||
tan, |
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.
Cleaner to just provide the checkpoint path I think?
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.
Hard disagree
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.
Offline: can either be checkpoint path or TAN object
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.
Offline: create #4
from experiments.train_scripts import GENERATORS | ||
from experiments.utils import parse_config_str | ||
from keras import backend as K | ||
from tanda.generator import ( |
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.
Is this import necessary?
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.
A bit cleaner than redefining IMO
Add a comment linking to said posts?
…On Tue, Dec 5, 2017 at 2:49 PM Henry Ehrenberg ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In experiments/cifar10/dataset.py
<#3 (comment)>:
>
def load_cifar10_batch(fpath, one_hot=True, as_float=True):
with open(fpath, 'rb') as f:
- data = cPickle.load(f)
+ data = cPickle.load(f, encoding='latin1')
Many stacks overflowed here. Just let it be.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#3 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABgw_WGumkIOb_HmQvXMN_gVZ2IN9CPkks5s9chrgaJpZM4Q3CYd>
.
|
Compatability imports Print parens Fix iters Fix imports
Keras interface and Python 3 compatability
Also adds a pretrained CIFAR-10 model