mslugocki / bayesfit

Bayesian Psychometric Curve Fitting Tool
Apache License 2.0
45 stars 8 forks source link

If the prior is off, I get a strange crash #3

Closed igordertigor closed 5 years ago

igordertigor commented 6 years ago

Here is a textfile I am using:

import numpy as np
import bayesfit as bf

data = [[0.1, 10, 20],
        [0.2, 11, 20],
        [0.3, 14, 20],
        [0.4, 17, 20],
        [0.5, 20, 20],
        [0.6, 19, 20]]
data = np.array(data)
data[:, 0] *= 10

metrics, options = bf.fitmodel(data,
                               priors=('Norm(.3,0.01)',
                                       None, None, None),
                               param_free=[True, True, True, False])

Note that I explicitly multiply my stimulus values to a range that really doesn't match with the prior. This gives the following error:

Traceback (most recent call last):
  File "test.py", line 16, in <module>
    param_free=[True, True, True, False])
  File "/home/ingo/tmp/bayesfit/.venv3/lib/python3.6/site-packages/bayesfit/bayesfit.py", line 137, in fitmodel
    metrics = _fit_Grid(data_copy, options)
  File "/home/ingo/tmp/bayesfit/.venv3/lib/python3.6/site-packages/bayesfit/bayesfit.py", line 185, in _fit_Grid
    metrics = _extract_metrics_grid(data, options, metrics, posterior, grid)
  File "/home/ingo/tmp/bayesfit/.venv3/lib/python3.6/site-packages/bayesfit/extractMetrics.py", line 243, in extract_metrics_grid
    metrics['MAP'][i] = grid['A'][map_index]
ValueError: setting an array element with a sequence.

Removing the multiplication or setting the prior to 'Norm(0.3, 0.01)' solves the problem and gives a reasonable looking function.

mslugocki commented 6 years ago

I apologize for the delayed response! Life got busy with moving ...

That is odd ... will look into the issue further in the coming days.

mslugocki commented 6 years ago

Given your example I have replicated the error.

There appears to be two issues here. One is the sequencing error, which still needs to be fixed.

The second is the prior set. A prior with a mean of 0.5 does not make sense for data whose intensities range between 1 - 10. Changing the mean value estimate to 5 [ 'Norm(5,1)' ] will appropriately fit a function, with a MAP value estimate for the mu of the normal distribution at 3.42, which makes sense. No errors are output from the example code below.

That code is:

metrics, options = bf.fitmodel(data, priors=('Norm(5,1)', None, None, None), param_free=[True, True, True, False])

sigmoidal

mslugocki commented 6 years ago

I re-read the issue you raise, and realize I missed that you explicitly set this prior to be off the mark in order to see what happens.

Will look into this further ...

igordertigor commented 6 years ago

@SlugocM , I'm not sure what you mean by the "sequencing error". Good luck with the move!

mslugocki commented 5 years ago

@igordertigor Thank you all around! Finally tracked down the error.

The probability vector returned for a prior that is way off the mark is an array of zeros. Of course, in theory these values are not zero, but are extremely small. However, in practice, they come back as zeros even if you use a double data type in an attempt to capture them.

Now, in the implementation, the vector representing the probabilities of a user provided prior are normalized before being applied to the likelihood surface to generate a posterior. Therefore, the vector of zeros was being divided by its sum, which is zero, resulting in an error that propagated forward... the details of which I will save. In short, it resulted in a sequence being returned instead of an array when computing one of the metrics from the posterior.

I have have updated the master branch with a fix that addresses this issue by checking whether the sum of the vector of probabilities for user defined priors is equal to zero. If so, an informative Value Error arises that tells the user that one of the priors they have provided is likely way off the mark, and so either to adjust the priors, or to use MLE by setting priors equal to None.

I will now close the issue :)

igordertigor commented 5 years ago

Sounds good. Thanks for looking into this.