Added support and test for mean of complex arrays #140
Conversation
src/tests/stats/test_mean_f03.f90
Outdated
| call assert( sum( abs( mean(d8,7) - sum(d8,7)/real(size(d8,7), dp) )) == 0.0_dp) | ||
| call assert( sum( abs( mean(d8,8) - sum(d8,8)/real(size(d8,8), dp) )) == 0.0_dp) | ||
|
|
||
| !cdp rank 8 | ||
| allocate(cd8(size(d,1), size(d,2), 3, 4, 5, 6, 7, 8)) | ||
| cd8 = cmplx(1._dp, 1._dp, kind=dp)*d8 | ||
|
|
||
| call assert( mean(cd8) - sum(cd8)/real(size(cd8), dp) == 0.0_dp) | ||
|
|
||
| call assert( sum( abs( mean(cd8,1) - sum(cd8,1)/real(size(cd8,1), dp) )) == 0.0_dp) | ||
| call assert( sum( abs( mean(cd8,2) - sum(cd8,2)/real(size(cd8,2), dp) )) == 0.0_dp) | ||
| call assert( sum( abs( mean(cd8,3) - sum(cd8,3)/real(size(cd8,3), dp) )) == 0.0_dp) | ||
| call assert( sum( abs( mean(cd8,4) - sum(cd8,4)/real(size(cd8,4), dp) )) == 0.0_dp) | ||
| call assert( sum( abs( mean(cd8,5) - sum(cd8,5)/real(size(cd8,5), dp) )) == 0.0_dp) | ||
| call assert( sum( abs( mean(cd8,6) - sum(cd8,6)/real(size(cd8,6), dp) )) == 0.0_dp) | ||
| call assert( sum( abs( mean(cd8,7) - sum(cd8,7)/real(size(cd8,7), dp) )) == 0.0_dp) | ||
| call assert( sum( abs( mean(cd8,8) - sum(cd8,8)/real(size(cd8,8), dp) )) == 0.0_dp) |
There was a problem hiding this comment.
It seems that the == 0.0_dp have not been changed to < dptol as in test_mean.f90. Could you do it, please?
src/stdlib_experimental_stats.fypp
Outdated
| #:def name(Rank, Type, Kind) | ||
| $:"mean_{0}_all_{1}{2}_{1}{2}".format(Rank, Type[0], Kind) | ||
| #:enddef |
There was a problem hiding this comment.
I like idea to use a preprocessing macro for the names of functions, especially if we will other functions with the same API (e.g., variance, median,...)
So, to make these macros more general, would it be possible to pass the name of the function, e.g.,:
#:def name(Varname, Rank, Type, Kind)
where Varname would be equal to mean, variance, median,....?
Also, would it be possible to add these macros in common.fypp, since these will be used for other functions?
There was a problem hiding this comment.
Yes, I'll do. I started that way but because the original routine names were complex, I decided for hard-code the names in order not to change too much the code. I'll amend and send a new request.
Also @jvdp1, could we make the functions pure? (I've not used much of submodules before, so I don't know if there is restrictions there)
There was a problem hiding this comment.
Functions in the submodule can be pure. However, it seems that the minimum required CMake does not support that (I am not sure if the last version of CMake supports it either; @zbeekma @certik @milancurcic ?).
Also the checks on the dimensions inside the functions may prohibit pure for some of them.
|
Ok, @jvdp1 @aradi What do you think of using a macro for names like the following? Assuming that t1='real', k1='sp', rank=1, |
|
I was getting an error for complex numbers. I am not sure but it seems
that it is not defined for complex numbers.
El 4/2/20 a las 14:53, Jeremie Vandenplas escribió:
… ***@***.**** approved this pull request.
Pending my minor comment, I think these changes are good. Having the
macro in |common.fypp| clarifies the other |.fypp| files. Thank you.
------------------------------------------------------------------------
In src/stdlib_experimental_stats_mean.fypp
<#140 (comment)>:
> ${t1}$, intent(in) :: x${ranksuffix(rank)}$
logical, intent(in), optional :: mask
${t1}$ :: res
if (.not.optval(mask, .true.)) then
- res = ieee_value(res, ieee_quiet_nan)
+ res = ieee_value(real(res, kind=${k1}$), ieee_quiet_nan)
|ieee_value| returns a NaN of the same type as |res|. So it should be OK
without the conversion to a |real|, right? Or do I miss something?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#140?email_source=notifications&email_token=AAOTPJPPWL24AYGAWDKEC4LRBGTTDA5CNFSM4KPXP4QKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUGRCUQ#pullrequestreview-353177938>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOTPJKF3PDVRP6JDB3SLNLRBGTTDANCNFSM4KPXP4QA>.
|
It seems that you are right: it is not defined for So it can be merged from my side. |
|
I resolved conflicts and tests seem to pass. We got two approvals, so I am going to merge it now. |
Added support and tests for complex types.
I changed the names of the specific functions, following the convention used in optval.