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

add custom relu implementation #92

Closed
wants to merge 8 commits into from
Closed

add custom relu implementation #92

wants to merge 8 commits into from

Conversation

akshay326
Copy link
Collaborator

@akshay326 akshay326 commented May 4, 2021

thanks to @matbesancon for #91, added a very trivial example of implementing ReLU as a layer in a NN trained on MNIST dataset using Flux.jl
There are some issues pending, specifically training on the complete dataset if very slow, and gives a JuMP warning:

Warning: The addition operator has been used on JuMP expressions a large number of times. 
This warning is safe to ignore but may indicate that model generation is slower than necessary. 
For performance reasons, you should not add expressions in a loop.
Instead of x += , use append!(x,y) to modify x in place. 
If y is a single variable, you may also use push!(x, coef, y) in place of x += coef*y

@akshay326
Copy link
Collaborator Author

also, current code works on latest Julia build (1.6.0). Seems Julia 1.0 supports only Tracker based Flux (while the latest one supports Zygote.jl)

@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #92 (e1e199e) into master (a2ab035) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #92   +/-   ##
=======================================
  Coverage   81.20%   81.20%           
=======================================
  Files           6        6           
  Lines         782      782           
=======================================
  Hits          635      635           
  Misses        147      147           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2ab035...e1e199e. Read the comment docs.

@matbesancon
Copy link
Collaborator

we should make this into a documentation page instead I think, it will also build it at every PR

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning suggests you are doing some arithmetic outside the JuMP macros. I couldn't see anything obvious here, so it must be somewhere inside DiffOpt.jl.

examples/custom-relu-mnist.jl Outdated Show resolved Hide resolved
examples/custom-relu-mnist.jl Outdated Show resolved Hide resolved
examples/custom-relu-mnist.jl Outdated Show resolved Hide resolved
examples/custom-relu-mnist.jl Outdated Show resolved Hide resolved
Co-authored-by: Oscar Dowson <odow@users.noreply.github.com>
@objective(
model,
Min,
x'x -2x'y[:, i]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the warning comes from I believe, this should be fixed by jump-dev/MutableArithmetics.jl#87

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved. seems there's a set_objective_coefficient method. + speeds up training a bit

@blegat
Copy link
Member

blegat commented May 5, 2021

we should make this into a documentation page instead I think, it will also build it at every PR

I agree, it would be helpful to run it at every commit with Literate

"""
relu method for a Matrix
"""
function myRelu(y::AbstractMatrix{T}; model = Model(() -> diff_optimizer(OSQP.Optimizer))) where {T}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matrix_relu better than myRelu? Also write out the model in the docstring

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing it

akshay326 and others added 2 commits May 6, 2021 06:18
Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>
@matbesancon
Copy link
Collaborator

status on this one? Should we merge it or make it a doc page first?

@akshay326
Copy link
Collaborator Author

@matbesancon should we close this (without merging ?)
#95 already contains the changes introduced in this PR

@matbesancon
Copy link
Collaborator

#95 will be lighter to review if this gets merged first.
Things missing:

  • Some comments / explanations
  • Adding it to the tests

@matbesancon
Copy link
Collaborator

otherwise there is a risk we have this example breaking at some point

@matbesancon
Copy link
Collaborator

bump @be-apt to add the example to CI

@akshay326
Copy link
Collaborator Author

closing this. duplicate code of #95

@akshay326 akshay326 closed this Jul 14, 2021
@akshay326 akshay326 deleted the flux-nn branch July 14, 2021 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants