search

diku-dk / bfast

GPU Implementation for BFAST
GNU General Public License v3.0
37 stars 17 forks source link

The error message of level does not reflect its actual behaviour #23

Closed 12rambau closed 3 years ago

12rambau commented 3 years ago

I read in the code that the level parameter of BFASTMonitor need to be in [0.95, 0.999]:

__critval_level = np.arange(0.95, 0.999, 0.001)

But I tried to launch it with value .95 I got the following error :

ValueError: ('level can only be one of', array([0.95 , 0.951, 0.952, 0.953, 0.954, 0.955, 0.956, 0.957, 0.958,
       0.959, 0.96 , 0.961, 0.962, 0.963, 0.964, 0.965, 0.966, 0.967,
       0.968, 0.969, 0.97 , 0.971, 0.972, 0.973, 0.974, 0.975, 0.976,
       0.977, 0.978, 0.979, 0.98 , 0.981, 0.982, 0.983, 0.984, 0.985,
       0.986, 0.987, 0.988, 0.989, 0.99 , 0.991, 0.992, 0.993, 0.994,
       0.995, 0.996, 0.997, 0.998, 0.999]))

Which make no sense because the value I've set up is prescisely 0.95.

I looked in your code and found out that the default value is .05... strange. I finally found my answer https://github.com/diku-dk/bfast/blob/4fd0c1727ec6ac309966d0ecfeef67e3ef02776b/bfast/monitor/utils.py#L465:

def compute_lam(N, hfrac, level, period):
    check(hfrac, period, 1 - level, "max")

    return get_critval(hfrac, period, 1 - level, "max")

You are not testing level but 1-level. Could you please change the error message which is very confusing ?

mortvest commented 3 years ago

Fixed on develop