argmin
argmin copied to clipboard
Adding ndarray implementations for ArgminRandom and ArgminMinMax
Adds missing trait implementations for ndarray inputs.
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 :)
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.
The remaining clippy errors are due to an update of clippy. You can simply add Eq to the derive as suggested in the logs.
Sure. I'll add a couple tests when I get some free time.
Sounds great! The fix for the clippy lints are now in main, therefore rebasing should fix the failing CI.
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.
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! :)