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

functions/ReLU,SoftMax: Restore functionality broken in #2528 #2532

Merged
merged 2 commits into from
Nov 11, 2022

Conversation

jvesely
Copy link
Collaborator

@jvesely jvesely commented Nov 11, 2022

PR #2528 introduced two breaking changes
1.) Renamed ReLU's derivative argument 'input'->'variable' which broke existing users of ReLU derivative.
2.) Changed SoftMax derivative calculation to use 'input' instead of 'output'. This is incorrect as the algorithm reuses existing outputs to calculate derivative results.

Both are restored to their original form. Moreover, a single input codepath is added to SoftMax,
recalculating results if they are not provided.
The compiled version and tests are adjusted accordingly.

This is the standard form for derivatives.

Signed-off-by: Jan Vesely <[email protected]>
Commit cae1465
	("llvm, functions/SoftMax: Implement compiled 'derivative' variant")
incorrectly assumed that the use of 'output' was an oversight.
It wasn't SoftMax derivative can take advantage of results if available.
This change restores the original functionality and adds a path to
compute the results if output is None.
This is used for testing where the results would need to be calculated
anyway.
The compiled variant is adapted in the same way, and the test are
updated to reflect the new results.

Signed-off-by: Jan Vesely <[email protected]>
@jvesely jvesely added the bug Should work but doesn't label Nov 11, 2022
@github-actions
Copy link

This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64):

No differences!

...

See CI logs for the full diff.

@jvesely jvesely added the regression Used to work but now it doesn't label Nov 11, 2022
@jvesely jvesely merged commit 4a40149 into PrincetonUniversity:devel Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Should work but doesn't regression Used to work but now it doesn't
Projects
Development

Successfully merging this pull request may close these issues.

1 participant