Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addition of corrected in API of var (following #149) #151

Merged
merged 4 commits into from
Feb 24, 2020

Conversation

jvdp1
Copy link
Member

@jvdp1 jvdp1 commented Feb 22, 2020

This PR is following the discussion in #149 .

New syntax

result = var(array [, mask[, corrected]])
result = var(array, dim [,mask[, corrected]])

  1. corrected is as last argument because mask is not always optional in the implementation (while corrected is always optional).

  2. corrected is used as follows, e.g.:

res = res / (n - merge(1._dp, 0._dp, optval(corrected, .true.)))

When mask is an array, the additional condition .and. n > 0._dp is needed to ensure that the result is NaN when all elements are .false. and corrected == .true., as specified in the spec.
If corrected == .true. and n==0, then we got something like res = res / -1, which may lead to a result equal to 0, e.g., in the following case if the additional condition is not added:

 res = sum((x - mean)**2, mask) / (n -&
                merge(1._${k1}$, 0._${k1}$, (optval(corrected, .true.)) .and. n > 0._${k1}$))

Tests have been adapted to consider this scenario.

I also added some tests for corrected==.false.. However, it is now only for real(sp). I can add more tests later in this PR when we will agree on its implementation.

@jvdp1 jvdp1 requested review from certik and milancurcic February 22, 2020 21:42
@jvdp1 jvdp1 marked this pull request as ready for review February 22, 2020 22:01
Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

That looks great, thanks for doing it!

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

Excellent, thank you Jeremie. I only have a few nit-picks to help improve readability. The implementation seems to have quite a few unnecessary explicit type casts.

First, the denominator n is declared as real (okay):

real(${k1}$) :: n

Because n is real, when getting the size of x, you don't need explicit cast here:

n = real(size(x, kind = int64), ${k1}$)

It can safely be just:

n = size(x, kind = int64)

Integer result of size() will be automatically promoted to the appropriate real kind.

Further, when calculating the variance:

res = sum((x - mean)**2) / (n - merge(1._${k1}$, 0._${k1}$, optval(corrected, .true.)))

x, mean, and the result of sum() are all real. We don't need to explicitly cast 1 and 0 in merge. Let the compiler do the work.

res = sum((x - mean)**2) / (n - merge(1, 0, optval(corrected, .true.)))

This would be correct even if n was integer, because the result of sum() in the enumerator is a real.

Otherwise, good to go!

@jvdp1
Copy link
Member Author

jvdp1 commented Feb 23, 2020

Thank you @milancurcic for your comments. I modified the code following them; a bad habit to lose.

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