-
Notifications
You must be signed in to change notification settings - Fork 19.7k
[Add Feature] - Throw an error if softmax is used with 1 neuron #18105
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
Closed
Closed
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
90c95b1
Add last layer activation check for softmax
Frightera 1cedb20
Split logic for sequential and functional models
Frightera 529f968
Add tests for _check_last_layer_activation
Frightera d1acddb
Update sequential check
Frightera 8363016
Update tests, logic and reformatting
Frightera ebf16c3
Update tests and the logic
Frightera afc156a
Make validate_softmax_activation experimental
Frightera 3a228fb
Fix edge case for _validate_softmax_output
Frightera e9c950e
Check the softmax axis and raise an error if relevant
Frightera 6355b23
Update softmax check tests
Frightera a6745ee
Minor typo fix
Frightera 92281f6
Fix test fails for _check_output_activation_softmax
Frightera 72a035f
Resolve conflict
Frightera 0af059c
Squashed commit master (merge) to resolve conflicts
Frightera 065cdea
Revert "Squashed commit master (merge) to resolve conflicts"
Frightera 446f1dd
Remove steps_per_execution_tuning from imports
Frightera 1fbd931
Fix lint
Frightera 2c867f8
Update TestCheckLastLayerActivation tests
Frightera File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 there any reason why we won't want to always validate the softmax activation?
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.
Actually no, however I thought it is better to have this something optional. The users who don't apply softmax on the last axis may trigger this warning. Maybe we can reflect this in warning message, something like:
You can disable this check by setting ...So it is something tricky to decide, what do you 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.
In general warnings can go unnoticed or ignored very easily; if we are confident based on the check that the user is making some mistake, I would simply error out. Are there known cases where the check fails but it's valid that the user would want to proceed?
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 should be no case(s) for users to proceed with that mistake. I'll extend this code to check the axis, and raising an error makes sense in that case. I'll update this by the end of the week.
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!
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.
Hi @rchao, sorry for the delay.
Now I updated the logic, we make sure that the user is making a mistake in this case and this throws an error rather than showing a warning.
I think it is better to add @fchollet as a reviewer since this can touch the model API interface.