Skip to content

Conversation

@jmh530
Copy link
Contributor

@jmh530 jmh530 commented Jun 1, 2020

This PR adds the overloads for sum that exist for prod and some of the other statistical functions already.

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2020

Codecov Report

Merging #264 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   90.19%   90.22%   +0.03%     
==========================================
  Files          52       52              
  Lines       10694    10706      +12     
==========================================
+ Hits         9645     9660      +15     
+ Misses       1049     1046       -3     
Impacted Files Coverage Δ
source/mir/math/numeric.d 89.24% <100.00%> (-0.12%) ⬇️
source/mir/math/stat.d 99.66% <100.00%> (ø)
source/mir/math/sum.d 97.11% <100.00%> (+0.62%) ⬆️

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 4cfc899...152cbd9. Read the comment docs.

}

///
sumType!(CommonType!T) sum(T...)(T r)
Copy link
Member

Choose a reason for hiding this comment

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

Why this overload is required?

@jmh530
Copy link
Contributor Author

jmh530 commented Jun 1, 2020 via email

@9il
Copy link
Member

9il commented Jun 2, 2020

I added two overloads as part of this PR. This one is for when you do sum(1, 2, 3) and also sum(1f, 2, 3). The other one is for when you do sum!float(1, 2, 3) or sum!float(1f, 2f, 3f). The main difference is that it means the user does not have to provide the type.

The following works well without a variadic template. It is very preferred to avoid variadic templates of functions, the generates a lot of template bloat and hard to maintain.

F sum(F)(scope F[] ar...)
{
    return ar[0];
}
void main() {
    auto s = sum(1f, 2, 3);
}

@jmh530 jmh530 force-pushed the jmh530-enhance-sum branch from 2a3a9e0 to 152cbd9 Compare June 2, 2020 10:45
@jmh530
Copy link
Contributor Author

jmh530 commented Jun 2, 2020

@9il Sounds good. I used scope const instead of just scope since you had recommended that previously.

One question I had as I was working on this is that I did not use core.lifetime.move for the typesafe variadic functions. Is this ok? I don't have a good sense of when that should be used (definitely with slice/range but otherwise?).

@9il
Copy link
Member

9il commented Jun 2, 2020

One question I had as I was working on this is that I did not use core.lifetime.move for the typesafe variadic functions. Is this ok? I don't have a good sense of when that should be used (definitely with slice/range but otherwise?).

Yes. But it would make sense to add move to fold if it hasn't been used before.

@9il 9il merged commit 6a946fe into libmir:master Jun 2, 2020
jmh530 added a commit to jmh530/mir-algorithm that referenced this pull request Jun 2, 2020
* Add variadic overloads to sum

* Add sum overloads

* Remove sum variadic template

* Remove prod/mean variadic templates
jmh530 added a commit to jmh530/mir-algorithm that referenced this pull request Jun 2, 2020
jmh530 added a commit to jmh530/mir-algorithm that referenced this pull request Jun 2, 2020
jmh530 added a commit to jmh530/mir-algorithm that referenced this pull request Jun 2, 2020
jmh530 added a commit to jmh530/mir-algorithm that referenced this pull request Jun 2, 2020
@jmh530 jmh530 deleted the jmh530-enhance-sum branch June 19, 2020 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants