-
Notifications
You must be signed in to change notification settings - Fork 58
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
Cover more priors with empirical distribution extension function #180
Conversation
Hopefully fixing #179 |
Codecov Report
@@ Coverage Diff @@
## master #180 +/- ##
==========================================
- Coverage 37.06% 36.96% -0.10%
==========================================
Files 20 20
Lines 3899 3931 +32
==========================================
+ Hits 1445 1453 +8
- Misses 2454 2478 +24
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to fix the handling of array-sampled parameters, specifically if there are single-sampled parameters later in the param
list.
@@ -77,8 +94,17 @@ def extend_emp_dists(pta, emp_dists, npoints=100_000, save_ext_dists=False, outd | |||
if emp_dist.param_name not in pta.param_names: | |||
continue | |||
param_idx = pta.param_names.index(emp_dist.param_name) | |||
prior_min = pta.params[param_idx].prior._defaults['pmin'] | |||
prior_max = pta.params[param_idx].prior._defaults['pmax'] | |||
if pta.params[param_idx].type not in ['uniform', 'normal']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am testing this with a run that has an array-sampled parameter and I'm get an error at this line. I believe it's the same issue that the full param_names
is longer than the list params
. In my case there is a single-sampled parameter in the list after the array-sampled parameter. I think that would cause this problem. This is why in #179 I made a separate list of param names from the parameter.Parameter.name
attributes.
The Trace of my error is below, if useful.
Traceback (most recent call last):
File "/mmfs1/home/hazboun/miniconda3/envs/pta/lib/python3.9/site-packages/pta_sim/scripts//model2a_advnoise_sw.py", line 196, in <module>
Sampler = sampler.setup_sampler(pta_crn, outdir=args.outdir, resume=True,
File "/mmfs1/home/hazboun/miniconda3/envs/pta/lib/python3.9/site-packages/enterprise_extensions/sampler.py", line 1107, in setup_sampler
jp = JumpProposal(pta, empirical_distr=empirical_distr, save_ext_dists=save_ext_dists, outdir=outdir)
File "/mmfs1/home/hazboun/miniconda3/envs/pta/lib/python3.9/site-packages/enterprise_extensions/sampler.py", line 252, in __init__
self.empirical_distr = extend_emp_dists(pta, self.empirical_distr, npoints=100_000,
File "/mmfs1/home/hazboun/miniconda3/envs/pta/lib/python3.9/site-packages/enterprise_extensions/sampler.py", line 97, in extend_emp_dists
if pta.params[param_idx].type not in ['uniform', 'normal']:
IndexError: list index out of range
Also, this PR probably needs a unit test. They could be tacked onto the other empirical distribution tests. |
Alright @AaronDJohnson getting closer! It looks like here is just a typo throwing things for a loop. I'll try and get a simple case set up for testing. Sorry I haven't yet. Traceback (most recent call last):
File "/mmfs1/home/hazboun/miniconda3/envs/pta/lib/python3.9/site-packages/pta_sim/scripts//model2a_advnoise_sw.py", line 196, in <module>
Sampler = sampler.setup_sampler(pta_crn, outdir=args.outdir, resume=True,
File "/mmfs1/home/hazboun/miniconda3/envs/pta/lib/python3.9/site-packages/enterprise_extensions/sampler.py", line 1113, in setup_sampler
jp = JumpProposal(pta, empirical_distr=empirical_distr, save_ext_dists=save_ext_dists, outdir=outdir)
File "/mmfs1/home/hazboun/miniconda3/envs/pta/lib/python3.9/site-packages/enterprise_extensions/sampler.py", line 258, in __init__
self.empirical_distr = extend_emp_dists(pta, self.empirical_distr, npoints=100_000,
File "/mmfs1/home/hazboun/miniconda3/envs/pta/lib/python3.9/site-packages/enterprise_extensions/sampler.py", line 97, in extend_emp_dists
short_par = '_'.join(emp_dist.param_namem.split('_')[:-1]) # make sure we aren't skipping priors with size!=None
AttributeError: 'EmpiricalDistribution1D' object has no attribute 'param_namem'
Traceback (most recent call last): |
fixed typo and added param switch
Oops! I thought it was merged already. I'll undo the delete and reopen the PR 👍 |
This adds support for Normal and Uniform priors to be covered automatically by empirical distributions. Additionally, I have added a skip if the empirical distribution is outside of these functions with a warning that asks the user to make sure they are covering their priors.
Priors with a size attribute should also be covered here (e.g. free spectral red noise models).