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

Use dictionaries for both loss and loss_weights consistently #500

Merged
merged 3 commits into from
Jun 16, 2021

Conversation

ronshapiro
Copy link
Contributor

No description provided.

loss_weights={
"priority": 1.0,
"department": 0.2,
},
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

@MarkDaoust MarkDaoust left a 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

loss_weights={
"priority": 1.0,
"department": 0.2,
},
Copy link
Collaborator

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.

Copy link
Contributor

@fchollet fchollet left a 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.

@ronshapiro
Copy link
Contributor Author

Done.

@MarkDaoust
Copy link
Collaborator

Thanks.

@fchollet fchollet merged commit b48a9a9 into keras-team:master Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants