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

Review examples #164

Merged
merged 46 commits into from
Dec 6, 2021
Merged

Review examples #164

merged 46 commits into from
Dec 6, 2021

Conversation

joaquimg
Copy link
Member

@joaquimg joaquimg commented Dec 4, 2021

replaces #95

@joaquimg
Copy link
Member Author

joaquimg commented Dec 4, 2021

@matbesancon should we keep:

"../examples/unit_example.jl"
"../examples/chainrules.jl"

?

I removed the custom SVM example with flux because it did not use DiffOpt, the derivatives were set to zero.

I might bring back "solve conic" without cvxpy and sensible results

@codecov
Copy link

codecov bot commented Dec 5, 2021

Codecov Report

Merging #164 (5d62b45) into master (56cb0e0) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 5d62b45 differs from pull request most recent head ec9b596. Consider uploading reports for the commit ec9b596 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
- Coverage   86.26%   86.14%   -0.13%     
==========================================
  Files           7        7              
  Lines         830      837       +7     
==========================================
+ Hits          716      721       +5     
- Misses        114      116       +2     
Impacted Files Coverage Δ
src/diff_opt.jl 87.60% <ø> (ø)
src/quadratic_diff.jl 99.17% <100.00%> (+0.05%) ⬆️
src/jump_moi_overloads.jl 62.85% <0.00%> (-1.91%) ⬇️

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 56cb0e0...ec9b596. Read the comment docs.

@matbesancon
Copy link
Collaborator

"../examples/unit_example.jl"
"../examples/chainrules.jl"

you mean given that we have a corresponding tutorial? Or do you think they are too redundant with one another?

@joaquimg
Copy link
Member Author

joaquimg commented Dec 6, 2021

you mean given that we have a corresponding tutorial? Or do you think they are too redundant with one another?

Yes, I think they are a bit redundant.
I would vote for keeping just chainrule_unit, the tutorial, that is a nice and complete example.

@joaquimg joaquimg requested a review from matbesancon December 6, 2021 14:34
@joaquimg
Copy link
Member Author

joaquimg commented Dec 6, 2021

Good to merge on my side.
If you like to chat about it let me know @matbesancon , we might be better talking offline if you think there are too many changes.

@matbesancon
Copy link
Collaborator

I would vote for keeping just chainrule_unit, the tutorial, that is a nice and complete example.

agreed on that yes it makes sense

@joaquimg joaquimg merged commit cb6f142 into master Dec 6, 2021
@matbesancon matbesancon mentioned this pull request Dec 6, 2021
@joaquimg joaquimg deleted the jg/ex branch December 7, 2021 20:28
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.

3 participants