finmath / finmath-lib

Mathematical Finance Library: Algorithms and methodologies related to mathematical finance.
Apache License 2.0
488 stars 168 forks source link

Change implementation of getQuantile to comply with JavaDoc documentation. #52

Closed cfries closed 6 years ago

cfries commented 6 years ago

The method getQuantile of RandomVariable does not comply with the JavaDoc of the interface RandomVariableInterface. The implementation should be changed to comply with the interpretation of returning x such that P(X < x) = alpha for a given alpha.

cfries commented 6 years ago

I would consider this a bug, since the implementation is not in line with the JavaDoc. Fixing it would changes the behaviour of getQuantile and hence this should be done under a move to a major reversion (3.x -> 4.x). However, since I see it as a bug, I prefer fixing it in 3.2.2 and make a corresponding remark in the release notes.

AlessandroGnoatto commented 6 years ago

Hi Christian,

Thanks for documenting the issue. The fix is quick and easy IMHO.

In RandomVariable line 333 instead of

int indexOfQuantileValue = Math.min(Math.max((int)Math.round((size()+1) * (1-quantile) - 1), 0), size()-1);

we need int indexOfQuantileValue = Math.min(Math.max((int)Math.round((size()+1) * (quantile) - 1), 0), size()-1);

Now if we run the following test application everything is nice IMHO:

import net.finmath.montecarlo.RandomVariable; import net.finmath.randomnumbers.MersenneTwister; import net.finmath.stochastic.RandomVariableInterface;

public class TestRandomVariable {

public static void main(String[] args) {
    MersenneTwister myTwist = new MersenneTwister(50);

    int numberOfPoints = 20000;

    double[] mySample = new double[numberOfPoints];

    for(int i = 0; i< numberOfPoints; i++) {
        double ithRandom = myTwist.nextDouble();
        mySample[i] = net.finmath.functions.NormalDistribution.inverseCumulativeDistribution(ithRandom);
    }

    RandomVariableInterface myRv = new RandomVariable(0.0,mySample);

    //95% quantile should be near 1.645
    System.out.println(myRv.getQuantile(0.95));
    //99% quantile should be near 2.33
    System.out.println(myRv.getQuantile(0.99));

}

}

cfries commented 6 years ago

Thanks Alessandro. Its already fixed and the unit test is there (committed it yesterday soon after I got your mail). Thanks to the students!

cfries commented 6 years ago

Fixed in 3.2.2.