search

baggepinnen / FluxOptTools.jl

Use Optim to train Flux models and visualize loss landscapes
MIT License
59 stars 4 forks source link

Rewrite test and fix small bugs #19

Closed mBarreau closed 1 year ago

mBarreau commented 1 year ago
baggepinnen commented 1 year ago

Hello Matthieu and thank you for this PR :)

I would like to ask if you can submit the PR without the automatic formatting applied, I do not like the way JuliaFormatter formats files so I do not use it. It's also a bit hopeless to review a PR that both makes changes and applies formatting to unrelated code.

mBarreau commented 1 year ago

@baggepinnen , does that look better to you?

mBarreau commented 1 year ago

@baggepinnen , what do you think now?

baggepinnen commented 1 year ago

Hi and thank you for updating the PR :) Here is some feedback

The change of keyword argument name from l to lnorm is a breaking change to the API, which feels unnecessary.

It's still the case that this PR has a huge diff, affecting a lot of things that aren't actually changed by the PR. For example, why was this large diff below made? Not only does it contribute unnecessary noise to the git history, it also prevents the "toggle comment" command in vscode from being able to toggle the block comment image

The PR also does a lot of completely unrelated things which is problematic since the commit history of the PR is quite messy at this stage, but a squash would make all these unrelated changes appear as a single commit which is not ideal either.

I appreciate that you want to contribute to this repository, but I would kindly ask you to avoid large unnecessary diffs that only change subjective things like code style etc., This holds for open-source projects in general, since your subjective opinions may not be shared by the repo owner, and commit history noise makes a lot of git tools much less useful. For example, each time a line of code is changed for no good reason, this git feature becomes useless image Instead of seeing the commit/PR that wrote the line, with a potentially helpful comment as to why the code behaves like it does, you now see an unrelated commit that just changed the style.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.05 :tada:

Comparison is base (02a8f03) 95.40% compared to head (40f6de3) 95.45%.

:exclamation: Current head 40f6de3 differs from pull request most recent head 98b3e73. Consider uploading reports for the commit 98b3e73 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #19 +/- ## ========================================== + Coverage 95.40% 95.45% +0.05% ========================================== Files 2 2 Lines 87 88 +1 ========================================== + Hits 83 84 +1 Misses 4 4 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `95.45% <80.00%> (+0.05%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Fredrik+Bagge+Carlson#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/baggepinnen/FluxOptTools.jl/pull/19?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Fredrik+Bagge+Carlson) | Coverage Δ | | |---|---|---| | [src/SLBFGS.jl](https://codecov.io/gh/baggepinnen/FluxOptTools.jl/pull/19?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Fredrik+Bagge+Carlson#diff-c3JjL1NMQkZHUy5qbA==) | `100.00% <ø> (ø)` | | | [src/FluxOptTools.jl](https://codecov.io/gh/baggepinnen/FluxOptTools.jl/pull/19?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Fredrik+Bagge+Carlson#diff-c3JjL0ZsdXhPcHRUb29scy5qbA==) | `90.69% <80.00%> (+0.22%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Fredrik+Bagge+Carlson). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Fredrik+Bagge+Carlson)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mBarreau commented 1 year ago

@baggepinnen , sure I will create a new PR which will be cleaner. Just one comment about the change in API. I perfectly understand and I would have left it as it was if possible. However, this parameter is used in plot and is conflicting with the line width... The behavior is now that it changes the line width of the contour together with the multiplication factor, which is not desirable I guess.