[GitHub] [commons-statistics] aherbert commented on pull request #26: Add truncated normal distribution.

[GitBox]

aherbert commented on pull request #26:
URL: https://github.com/apache/commons-statistics/pull/26#issuecomment-782818168


   I agree with the parameterised tests. This is a change that can be 
implemented later. We can discuss the approach on the mailing list.
   
   I think the `TOO_LARGE` and `TOO_SMALL` apply when one of the arguments is 
fixed and the other is incorrect relative to it. The case here is for a range 
that is not specified correctly. So perhaps we should use an `INVALID_RANGE` 
exception where the upper bound is not above the lower bound:
   
   `Lower bound %s is not below the upper bound %s`
   
   This allows you to specify the range arguments in their declared order:
   
   ```java
   if (upper <= lower) {
  throw new DistributionException(DistributionException.INVALID_RANGE, 
lower, upper);
   }
   ```
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [commons-statistics] aherbert commented on pull request #26: Add truncated normal distribution.

[GitBox]

aherbert commented on pull request #26:
URL: https://github.com/apache/commons-statistics/pull/26#issuecomment-782051472


   Thanks for the contribution. The implementation and tests look good (all 
edge cases are covered).
   
   The code is currently failing PMD checks. Most of these are for missing the 
`final` keyword on local variables. You can see the PMD errors by running:
   ```
   mvn pmd:pmd
   ```
   Then open the file in `target/site/pmd.html`
   
   There is also an error for a confusing ternary. I think it is OK in this 
context but to avoid the issue what do you think to rewriting the constructor 
as:
   ```java
   if (lower == Double.NEGATIVE_INFINITY) {
   if (upper == Double.POSITIVE_INFINITY) {
   // No truncation
   this.mean = mean;
   variance = sd * sd;
   } else {
   // One-sided lower tail truncation
   double betaRatio = pdfBeta / cdfBeta;
   this.mean = mean - sd * betaRatio;
   variance = sd * sd * (1 - beta * betaRatio - betaRatio * 
betaRatio);
   }
   } else {
   if (upper == Double.POSITIVE_INFINITY) {
   // One-sided upper tail truncation
   double alphaRatio =  pdfAlpha / cdfDelta;
   this.mean = mean + sd * alphaRatio;
   variance = sd * sd * (1 + alpha * alphaRatio - alphaRatio * 
alphaRatio);
   } else {
   // Two-sided truncation
   this.mean = mean + pdfCdfDelta * parentSd;
   variance = sd * sd * (1 + alphaBetaDelta - pdfCdfDelta * 
pdfCdfDelta);
   }
   }
   ```



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org