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

Empirical Distribution Check does not handle Multicomponent or Non-Uniform Parameters #179

Open
Hazboun6 opened this issue Apr 25, 2022 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@Hazboun6
Copy link
Member

The new code in enterprise_extensions.sampler.extend_emp_dists only handles parameters that have pmin and pmax kwargs. While our main searches use these types of parameters enterprise natively supports Normal parameters and user defined parameters. I found the issue when trying to add empirical distributions for an ACE_SWEPAM parameter of size=24. Since this particular parameter is from data, hence it has a natural min and max, other parameters, like the Normal parameters used for DMEFAC or nonlinear timing parameters, do not.

The code also does not handle parameters that have a size attribute not equal to None. For these parameters the dimensions of pta.params is shorter than that of pta.param_names, so the code errors out on an IndexError.

@Hazboun6 Hazboun6 added the bug Something isn't working label Apr 25, 2022
@AaronDJohnson
Copy link
Collaborator

Do we have any ideas on the best way to deal with this? This also affects the make_empirical_distr function. I'm thinking maybe that it would be best to let the user handle these issues if they can't be handled automatically. Just add a warning letting them know that it can't be handled automatically....

@Hazboun6
Copy link
Member Author

Hazboun6 commented Apr 26, 2022

So I think the issue with parameters that have a size, i.e. get sampled as an array, is a bug. I've come up with a way to fix it in my repo, but curious what you think. Below is a snippet from the function. Basically, you have to get the list of par names from the params list, rather than param_names, and then search for the pmin/pmax.

param_names = [par.name for par in pta.params]
    for emp_dist in emp_dists:
        if isinstance(emp_dist, EmpiricalDistribution2D) or isinstance(emp_dist, EmpiricalDistribution2DKDE):
            # check if we need to extend the distribution
            prior_ok=True
            for ii, (param, nbins) in enumerate(zip(emp_dist.param_names, emp_dist._Nbins)):
                if param not in pta.param_names:  # skip if one of the parameters isn't in our PTA object
                    continue

                # check 2 conditions on both params to make sure that they cover their priors
                # skip if emp dist already covers the prior
                try: # Assume single parameter
                    param_idx = param_names.index(param)
                    prior_min = pta.params[param_idx].prior._defaults['pmin']
                    prior_max = pta.params[param_idx].prior._defaults['pmax']
                except IndexError: # If param with size then pull off name.
                    short_par = '_'.join(param.split('_')[:-1])
                    param_idx = param_names.index(short_par)
                    prior_min = pta.params[param_idx].prior._defaults['pmin']
                    prior_max = pta.params[param_idx].prior._defaults['pmax']

This is then repeated anywhere prior_min/prior_max were obtained in the older code.

@Hazboun6
Copy link
Member Author

The issue of infinite range priors is harder. I do like your idea. Maybe it would be best to just warn people that the distributions are not covering the prior, and they should remake them.

What would be the proper way to deal with a Normal prior?

@AaronDJohnson
Copy link
Collaborator

AaronDJohnson commented Apr 26, 2022

What would be the proper way to deal with a Normal prior?

Covering some reasonable range of values (perhaps like mu +/- 10 * sigma) should essentially "cover" the prior in that case. I'm not sure that we can do much better than that. There is a small possibility of getting something outside of the prior, but it won't happen often enough to make a significant difference in the chains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants