Skip to content

Conversation

@pberkes
Copy link
Collaborator

@pberkes pberkes commented Nov 15, 2024

Fix #92 , see also #152

This PR adds the "posterior mean" estimate of the parameters.

When we discussed this, we had agreed on removing the possibility of choosing the estimate type, and add the new estimate in parallel in the Result object.

This works to some extent, but the it means that the "mean" estimate cannot be used in plots, etc.

What I did is that it's now possible to specify the default estimate type to use, but then one can override it in all plots and Result methods by setting the estimate_type optional parameter.

E.g.
psp.plot_psychometric_function(result) -> uses the default type set in Configuration
psp.plot_psychometric_function(result, estimate_type='mean') -> uses "mean", ignores Configuration
psp.plot_psychometric_function(result, estimate_type='MAP') -> uses "MAP", ignores Configuration

You can have a look at the parameters_estimate.ipynb notebook to see this implementation in action.

It looks like a lot of changes, but if you look at it commit by commit is should be easy to follow

@pberkes
Copy link
Collaborator Author

pberkes commented Nov 15, 2024

The addition of the "mean" estimate is only a few lines, in _fit_parameters . The rest is changes to make is possible to use "mean" or "MAP", and tests

@guillermoaguilar
Copy link
Collaborator

This works to some extent, but the it means that the "mean" estimate cannot be used in plots, etc.

I'm not sure if I follow, is this because it hasn't been saved to the results dictionary?

@pberkes
Copy link
Collaborator Author

pberkes commented Nov 20, 2024

This works to some extent, but the it means that the "mean" estimate cannot be used in plots, etc.

I'm not sure if I follow, is this because it hasn't been saved to the results dictionary?

I did implement the extra mean estimate dictionary in Results, but we still need to have a way for the plots, Results.slope etc. to use one estimate or another (e.g. plotting the estimated sigmoid with MAP estimate or mean estimate). That is what most changes are about.

@otizonaizit
Copy link
Collaborator

LGTM! Thanks @pberkes this is a lot of work and it looks very good and useful to me. I'd like to merge this right away and sorry for letting it sit for so long.
One nit-pick though. When running the tests I get this:

[...]
===================warnings summary ==========================
psignifit/tests/test_param_recovery.py::test_mean_vs_map_estimate
  [...]/python-psignifit/psignifit/psignifit.py:162: UserWarning: All provided data blocks contain <= 5 trials.
              Did you sample adaptively?
              If so please specify a range which contains the whole psychometric function in
              conf.stimulus_range.
              An appropriate prior prior will be then chosen. For now we use the standard
              heuristic, assuming that the psychometric function is covered by the stimulus
              levels, which is frequently invalid for adaptive procedures!
    warnings.warn("""All provided data blocks contain <= 5 trials.

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

Could you fix this warning @pberkes and then merge directly if you are faster than me?

@pberkes pberkes merged commit f198fac into wichmann-lab:main Nov 27, 2024
10 checks passed
@pberkes pberkes deleted the add-mean-estimate branch November 27, 2024 08:54
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.

support estimate types different from MAP?

3 participants