-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use dictionaries for both loss
and loss_weights
consistently
#500
Conversation
guides/functional_api.py
Outdated
loss_weights={ | ||
"priority": 1.0, | ||
"department": 0.2, | ||
}, |
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 was wondering how it lines up the outputs to the losses given that the outputs are defined as: outputs=[priority_pred, department_pred]
But now I see that it's using the layer names.
Should there be an example using outputs={'priority': priority_pred, 'department': department_pred}
? I find that form is always clearer for the user of a multiple output model.
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.
Updated the description text so it's clearer where these names come from.
I don't know what's better, specifying the names in the layer or in the model.fit() call. I leave that to you.
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.
Regardless of that Idea, I think this is a good change. Let's get it merged.
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.
One last think, I should have mentioned on the first pass:
Could you run the script to update the generated files, and add those to the PR as well?
I believe the command is (from the scripts/
directory):
python autogen.py add_guide functional_api.py
guides/functional_api.py
Outdated
loss_weights={ | ||
"priority": 1.0, | ||
"department": 0.2, | ||
}, |
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.
Regardless of that Idea, I think this is a good change. Let's get it merged.
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, please replicate the fix in the md and ipynb files.
Done. |
Thanks. |
No description provided.