Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add elicitation of Beta distribution with bounds and mode #309

Merged
merged 13 commits into from
Feb 14, 2024

Conversation

nishant42491
Copy link
Contributor

@nishant42491 nishant42491 commented Feb 3, 2024

Description

Checklist

  • Code style is correct (follows pylint and black guidelines)
  • Includes new or updated tests to cover the new feature
  • New features are properly documented (with an example if appropriate)

Copy link
Contributor

@aloctavodia aloctavodia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few files under the .idea folder that should not be part of this PR

demo.py Outdated Show resolved Hide resolved
@aloctavodia
Copy link
Contributor

As you can see for the Beta case, the maxent function already does something similar and in my opinion in a much more flexible way. Anyway, it may be the case that someone prefers this method. In that case, this function should have the l1, l2 and mode as input values and it should return a pz.Beta distribution with the fitted parameter and optionally plot the result. Probably the eps can be fixed as 0.005.

Check pz.maxent for a reference of parameters names and other details

@aloctavodia
Copy link
Contributor

Thanks!

I made a few changes. Check maxent as a reference for further changes, like adding a docstring and refactoring. Notice that all optimization routines have to be at interval/optimization.py

The main changes are that the signature is (lower, upper, mode, mass=0.9, plot=True, plot_kwargs=None, ax=None), meaning the mode is a mandatory argument, together with the lower and upper bounds.

Also, notice that it returns a frozen pz.Beta() distribution, meaning we have already passed the parameters.

To compute the cdf I did a little trick. First I call pz.Beta(.,.) but then I use the _parametrization, this is slightly faster. Instantiating a distribution in PreliZ (and SciPy) can be relatively costly.

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2024

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (9a68697) 84.19% compared to head (e392f1e) 83.61%.

Files Patch % Lines
preliz/unidimensional/beta_mode.py 18.51% 22 Missing ⚠️
preliz/internal/optimization.py 11.11% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #309      +/-   ##
==========================================
- Coverage   84.19%   83.61%   -0.58%     
==========================================
  Files          35       36       +1     
  Lines        4181     4218      +37     
==========================================
+ Hits         3520     3527       +7     
- Misses        661      691      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aloctavodia
Copy link
Contributor

BTW, notice that we are slightly departing here from what is is presented in the paper in the sense that we are demanding the user to provide a mode and allowing them to select the mass (instead of fixing it at 0.99).

Ohh another think still missing is that we should check that the mode is between lower and upper, otherwise raise an error

@nishant42491
Copy link
Contributor Author

Thanks!

I made a few changes. Check maxent as a reference for further changes, like adding a docstring and refactoring. Notice that all optimization routines have to be at interval/optimization.py

The main changes are that the signature is (lower, upper, mode, mass=0.9, plot=True, plot_kwargs=None, ax=None), meaning the mode is a mandatory argument, together with the lower and upper bounds.

Also, notice that it returns a frozen pz.Beta() distribution, meaning we have already passed the parameters.

To compute the cdf I did a little trick. First I call pz.Beta(.,.) but then I use the _parametrization, this is slightly faster. Instantiating a distribution in PreliZ (and SciPy) can be relatively costly.

Thanks for making all the changes. I'll check the maxent reference to update all the docstrings for it

@nishant42491
Copy link
Contributor Author

BTW, notice that we are slightly departing here from what is is presented in the paper in the sense that we are demanding the user to provide a mode and allowing them to select the mass (instead of fixing it at 0.99).

Ohh another think still missing is that we should check that the mode is between lower and upper, otherwise raise an error

should i include an assert statement to the check the bounds of the mode?

@nishant42491
Copy link
Contributor Author

I have added docstrings and have tried refactoring the code to follow the standards set in the maxent function. I have also added an extra parameter eps instead of hard setting the tolerance level to 0.005.

@nishant42491
Copy link
Contributor Author

I'm not sure what optimisations are exactly required for the function. Also where would this module exist inside of the preliz structure? maybe somewhere in the pz.unidimentional directory and what should be the name of the module and the function one_iter exactly?

Copy link
Contributor

@aloctavodia aloctavodia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, a few more comments. Also we need to think about a name for the function

demo.py Outdated Show resolved Hide resolved
demo.py Outdated Show resolved Hide resolved
demo.py Outdated Show resolved Hide resolved
demo.py Outdated Show resolved Hide resolved
demo.py Outdated Show resolved Hide resolved
demo.py Outdated Show resolved Hide resolved
demo.py Outdated Show resolved Hide resolved
@nishant42491
Copy link
Contributor Author

@aloctavodia thanks for all your ideas. I have tried to clean up the code by following your comments. I cannot think of a function name though the closest thing I can think of is beta_bounds_elicitation. I have added an optimisation routine let me know if that is what you were looking for. additionally, I have tried to make the plotting routine resemble the one in the maxent function however when we keep the default argument as None for the plot kwargs arg will raise an error as it expects it to be a mapping.
I have also added the tau calculation routine which you had suggested to make our function run faster.

@aloctavodia aloctavodia changed the title Adding a Dirichlet Prior Add eliciation of Beta distribution with bounds and mode Feb 12, 2024
@aloctavodia
Copy link
Contributor

aloctavodia commented Feb 12, 2024

A test is needed. We can add it as part of this PR, or in a different one, after merging this one.

Make default 0.94 to be consistent library-wise
@aloctavodia aloctavodia changed the title Add eliciation of Beta distribution with bounds and mode Add elicitation of Beta distribution with bounds and mode Feb 12, 2024
@nishant42491
Copy link
Contributor Author

@aloctavodia I think maybe we can let the tests be a separate PR. Do you need anything else to be done as part of this PR?

@aloctavodia aloctavodia merged commit d6903f0 into arviz-devs:main Feb 14, 2024
4 checks passed
@aloctavodia
Copy link
Contributor

Thanks @nishant42491!

@aloctavodia aloctavodia mentioned this pull request Feb 14, 2024
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.

None yet

3 participants