-
Notifications
You must be signed in to change notification settings - Fork 19.7k
Enable element-wise weighing of outputs in model.fit(..., sample_weight=weights) #10775
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
Conversation
|
I have asked for guidance in the thread "How to debug a keras pull request" in the mailing list. |
…work.errors_impl.InvalidArgumentError: Incompatible shapes
Some text lines were too long
|
@pavithrasv what's your take on this feature? |
|
Thank you for the PR @rcasero. Will review soon. |
Dapid
left a 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.
Looks great! We need some tests, though.
|
I think this feature is very interesting, and I will be using it a lot. My use case is predictions of some experimental results. When I am predicting 1D features, I can use "temporal" mode and mask out the points where I am missing data, but when I am predicting 2D, I can no longer use that trick. My solution now is to wrap my model and manually force the outputs at the missing pixels to 0; essentially multiplying the outputs by the "present" mask. This mode would allow me to have simpler models, avoid the extra complication of extracting the inner model when I need to use it, and also use the same structure for 1 and 2D. |
|
I wrote a test script, but I'd need help to turn it into test units. (As I mention in the Keras API Design Review google doc, I've tried following the instructions to test keras, but it fails for me even with the unpatched main keras branch.) |
|
I can see this feature being useful for use cases like the one @Dapid has mentioned. It could also be used to mask unknown elements in a sample. Did a first pass through the code, will review again after unit tests have been added. |
|
Suggestions by @pavithrasv pushed in commit 157244c |
|
I implemented this feature because I generate training data with unknown elements, as @pavithrasv mentions. Training a network this way is working for me. As mentioned above, I have a testing script, but I don't know how to generate unit tests in keras. |
|
@pavithrasv Could you please take a look at the recent changes? Thank you. |
pavithrasv
left a 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.
Thank you for the changes. Can you add unit tests to test_training.py :https://github.com/keras-team/keras/blob/master/tests/keras/engine/test_training.py
|
@pavithrasv What I see in "View changes" proposed by you is the patch I've submitted, right? Do I need to do anything about that? Roger about the unit tests. |
|
Yes, the only change I had requested after that commit were the unit tests. Thank you! |
|
@pavithrasv I've found an error if one has two outputs, and one is e.g. import os
os.environ['KERAS_BACKEND'] = 'tensorflow'
import keras
import keras.backend as K
import numpy as np
from keras.models import Model, Sequential
from keras.layers import Activation, Conv2D, Input
from keras.layers.normalization import BatchNormalization
from keras.utils import multi_gpu_model
# remove warning "Your CPU supports instructions that this TensorFlow binary was not compiled to use: AVX2 FMA"
os.environ['TF_CPP_MIN_LOG_LEVEL'] = '2' # Just disables the warning, doesn't enable AVX/FMA
# image_data_format = 'channels_first'
image_data_format = 'channels_last'
K.set_image_data_format(image_data_format)
# simulate input images
im = np.zeros(shape=(10, 64, 64, 3), dtype='uint8')
# simulate network output
out = 2 * np.ones(shape=(10, 64, 64, 1), dtype='float32')
aux_out = 5 * np.ones(shape=(10, 22, 22, 1), dtype='float32')
# simulate training weights for network output
# weight = np.ones(shape=(10, 64, 64, 1), dtype='float32')
weight = np.ones(shape=(10, 64, 64, 1), dtype='float32')
aux_weight = np.ones(shape=(10, 22, 22, 1), dtype='float32')
# simulate validation data
im_validation = 3 * np.ones(shape=(5, 64, 64, 3), dtype='uint8')
out_validation = 4 * np.ones(shape=(5, 64, 64, 1), dtype='float32')
validation_data = (im_validation, out_validation)
# optimizer
optimizer = keras.optimizers.SGD(lr=0.01, decay=1e-6, momentum=0.9, nesterov=True)
'''Multi-output CNN with outputs of different number of features
'''
# create network model
input = Input(shape=im.shape[1:], dtype='float32')
x = Conv2D(filters=32, kernel_size=(3, 3), strides=1, padding='same')(input)
x = BatchNormalization(axis=3)(x)
x = Activation('relu')(x)
main_output = Conv2D(filters=1, kernel_size=(1, 1), strides=1, padding='same', name='main_output')(x)
aux_output = Conv2D(filters=2, kernel_size=(1, 1), strides=3, padding='same', name='aux_output')(x)
model = Model(inputs=input, outputs=[main_output, aux_output])
'''list format (sample_weight_mode=['element', 'element'])
'''
model.compile(loss='mae', optimizer=optimizer, metrics=['accuracy'],
sample_weight_mode=['element', 'element'])
model.fit(im, [out, np.repeat(aux_out, repeats=2, axis=3)],
sample_weight=[weight, np.repeat(aux_weight, repeats=2, axis=3)],
batch_size=3, epochs=3) |
|
I think something is going wrong around those lines: # reduce weight array to same ndim as score_array (needed for
# sample_weight_mode='element')
if weight_ndim > K.ndim(score_array):
weights = K.reshape(weights, K.shape(score_array))If you do |
|
Thanks @gabrieldemarmiesse . You were right, the problem was there. Furthermore, the patch had a conceptual error, because the element-wise weights should have the size of I still need to do some tests and write the test units. |
|
My last commits broke the 10782.8 test, Python: 2.7, KERAS_BACKEND=cntk in the Travis CI build, in particular Can't debug for the moment, because keras docker requires On a related note, I had to:
docker build -t keras --build-arg python_version=$(PYTHON_VERSION) ...
with and replacing with because otherwise the |
|
The error is not due to your commit. It is a flaky test. I'll restart the build for you. You can also install keras cpu by folowing the instructions in |
|
Thanks, @gabrieldemarmiesse . I'll look at |
|
I'll try to fix the test. Please use |
|
I can't review this anymore since I added some commits. We need new reviewers. The build is passing, so it's ready for review. |
|
Thanks. Anything else that needs to be done? Do we need that error message if the weights ndim is not 1 less than the output's ndim? |
|
We need this error message. You already wrote it. It looks like this now: if sample_weight is not None and sample_weight.shape != score_array_shape:
raise ValueError('Found a `sample_weight` array with shape ' +
str(sample_weight.shape) +
' for output with shape ' +
str(y.shape) +
'. When sample_weight_mode="element", ' +
'weights and score_array must have the same size.'
'Your `sample_weight` array should have the '
'following shape: ' + str(score_array_shape)) |
|
I think we just need to wait for the build, and if it passes, for a review from another member of the keras team. Thanks for your work @rcasero ! |
|
Thanks for your help, @gabrieldemarmiesse |
|
@pavithrasv, this PR is ready. Could you please take a look? |
|
@pavithrasv Just bumping this up |
|
@fchollet please take a look at it when you have the time. |
The tests have been added.
|
@fchollet @gabrieldemarmiesse bumping this up, as it seems to have been forgotten |
|
I haven't forgotten, but each PR changing the API needs @fchollet 's approval. We need to be better organised for this. |
|
@gabrieldemarmiesse @fchollet Not blaming anyone. :) I'd just love this to be merged so that updates to keras don't keep breaking the branch, and we can refer to it in a publication. I've just merged the official keras into my branch, as there were some new conflicts. Now it passes the tests again. |
|
@todiketan Perhaps you could ask that question in the thread you mention (just reply to my message there), as it's off-topic here? (This is an issue about adding an element-wise weighting feature to keras). |
|
Thank you for preparing this PR. We are no longer adding new features to multi-backend Keras, as we are refocusing development efforts on |
|
@fchollet Thanks for letting me know. Could you give me a couple of pointers of where the code goes within https://github.com/tensorflow/tensorflow/tree/master/tensorflow/python/keras? The code structure seems to be different to regular keras, although the filenames are the same. The PR makes the following changes:
|
Summary
Add element-wise weighting of the loss function.
Described in Issue #10561
Keras API Design Review google doc with comments enabled here
https://docs.google.com/document/d/19BDXgNmeTgpgb9xYKzNboXyM7XX2PeM3mlvCFCdIQj0/edit?usp=sharing
Related Issues
None, as far as I know.
PR Overview
As described in the API Design Review, I've had some trouble with this, and could use some guidance.
Help added in the code, but as noted in the API Design Review doc, I also need a bit of guidance with that.
I don't know how to test this.
It adds a new possible value
'element'to the optionsample_weight_modeinmodel.compile().