argmin icon indicating copy to clipboard operation
argmin copied to clipboard

Adding ndarray implementations for ArgminRandom and ArgminMinMax

Open hypotrochoid opened this issue 3 years ago • 5 comments

Adds missing trait implementations for ndarray inputs.

hypotrochoid avatar Jul 15 '22 04:07 hypotrochoid

Thanks for the contribution! Could you have a look at the clippy lints and run rustfmt, please? Other than that it looks good to me :)

stefan-k avatar Jul 15 '22 06:07 stefan-k

On other thing I noticed: Could you add tests, please? I know that the other backends are also missing tests, but this could be a good place to start testing this ;) One thing I could imagine in case of ArgminRandom is checking that the values are within the provided bounds.

stefan-k avatar Jul 15 '22 06:07 stefan-k

The remaining clippy errors are due to an update of clippy. You can simply add Eq to the derive as suggested in the logs.

stefan-k avatar Jul 15 '22 15:07 stefan-k

Sure. I'll add a couple tests when I get some free time.

hypotrochoid avatar Jul 21 '22 04:07 hypotrochoid

Sounds great! The fix for the clippy lints are now in main, therefore rebasing should fix the failing CI.

stefan-k avatar Jul 22 '22 13:07 stefan-k

Codecov Report

Base: 91.67% // Head: 91.50% // Decreases project coverage by -0.16% :warning:

Coverage data is based on head (65bf067) compared to base (caab45a). Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
- Coverage   91.67%   91.50%   -0.17%     
==========================================
  Files         116      117       +1     
  Lines       17601    17632      +31     
==========================================
  Hits        16135    16135              
- Misses       1466     1497      +31     
Impacted Files Coverage Δ
argmin-math/src/ndarray_m/random.rs 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

codecov-commenter avatar Dec 09 '22 07:12 codecov-commenter

I rebased this commit and removed the ndarray ArgminMinMax implementation in favor of the one which was added to main since this PR was opened. I also implemented the trait directly for all basic types instead of using traits because this causes fewer problems down the line in my experience.

I am merging this PR despite the missing tests which will be added later.

Thanks again for this contribution! :)

stefan-k avatar Dec 09 '22 08:12 stefan-k