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

Code Review 8/29/18 #46

Open
wants to merge 231 commits into
base: empty
Choose a base branch
from
Open

Code Review 8/29/18 #46

wants to merge 231 commits into from

Conversation

auroracramer
Copy link
Collaborator

No description provided.

justinsalamon and others added 30 commits October 11, 2017 10:53
Conflicts:
	l3embedding/train.py
Add basic functions to sample from audio and video files
Conflicts:
	l3embedding/train.py
Move script part of train.py to a different file
Add retry loop to opening video files
Add periodic model checkpoints to training
Custom read_video function
Fix model building configuration
Add augmentation for audio and images from L3 paper and add use of validation set
* Move ReLU _after_ batch norm
* Use weight decay (assumed L2)
* Set learning rate to 1e-4
* Allow for volume gain less than 0dB, as paper seems like it should allow for that
Add more changes to the model so that it is consistent with paper
auroracramer and others added 24 commits April 20, 2018 18:19
…on data, and fix bug where only best parameter results are updated in the metrics history
train_metrics = compute_metrics(y_train, train_pred, num_classes=num_classes)
# Set up train and validation metrics
train_metrics = {
'loss': metric_cb.train_loss[-1],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we're using early stopping, we should find the index of the lowest validation loss (as opposed to the last index) and report those metrics!

This will likely change some of the results...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, we need to make sure we're reloading the checkpoint with the best loss, since Keras just leaves the model in whatever state the model was when finished training

'accuracy': metric_cb.train_acc[-1],
'class_accuracy': train_metrics['class_accuracy'],
'average_class_accuracy': train_metrics['average_class_accuracy']
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should also keep track of training and validation accuracies over time, even if it is saved in the history CSV

valid_metrics = {
'loss': metric_cb.valid_loss[-1],
'accuracy': metric_cb.valid_acc[-1],
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same thing as above two comments

train_data_skf = train_data
valid_data_skf = valid_data
else:
splitter = StratifiedShuffleSplit(n_splits=1, test_size=valid_ratio)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a code comment, but we should make sure in the paper we specify that we do a stratified split

if y.ndim == 2:
y = np.argmax(y, axis=1)
if pred.ndim == 2:
pred = np.argmax(pred, axis=1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Convert y and pred to np.array just in case a list is passed in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This shouldn't actually have affected anything since we're calling arr.ndim, but it's a good precaution

@@ -0,0 +1,128 @@
name: l3embedding
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should update this

# 257 x 199 x 1
y_a = Spectrogram(n_dft=n_dft, n_hop=n_hop, power_spectrogram=1.0, # n_win=n_win,
return_decibel_spectrogram=True, padding='valid')(x_a)
y_a = BatchNormalization()(y_a)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Take this BN out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or leave it in, depending on what we decide

####
# INPUT
x_i = Input(shape=(224, 224, 3), dtype='float32')
y_i = BatchNormalization()(x_i)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Take this BN out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or leave it in, depending on what we decide.

self.best_valid_loss, self.best_train_acc, self.best_valid_acc]

update_experiment(self.service, self.spreadsheet_id, self.param_dict,
'R', 'AA', values, 'embedding')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be 'Z', though it doesn't seem to be breaking anything.

else:
m, inputs, outputs = MODELS[model_type](num_gpus=gpus)

loss = 'binary_crossentropy'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The loss is the same as as categorical_crossentropy, up to a constant. But binary_crossentropy = 2 * categorical_crossentropy. This shouldn't cause any serious optimization issues, but effectively halves the weight decay constant.

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