Burst

This branch has a sine Gaussian signal model and a supernova signal model with example python scripts.

Hi @jade.powell ,

Looks great and will be cool to have a non-CBC example, thanks :).

I have one minor worry. It looks like you added some default priors, but the names are not too descriptive, e.g. coeff1. I realise that the names that are in there already (for CBC signals) also aren't very descriptive. Do you think it would be possible to make the names more descriptive, or are they supposed to be vague in this way? In either case, maybe we could add some short comment to that effect?

Other than that, by the way, I'm happy to merge.

Hi @gregory.ashton, their "official" name is principal component coefficients, which is why I called them coeff. I could change it to pc_coeff? Do you have any other suggestions?

Hi Jade,

Ah okay yes that would make more sense. I just read a bit more (sorry for not putting this in the first comment).

I also noticed that you are setting up the defaults in the sine Gaussian case like this

priors['Q'] = tupak.prior.create_default_prior(name='Q')
priors['frequency'] = tupak.prior.create_default_prior(name='frequency')

and similarly for the supernova example. Since you have made them default priors, I think that you shouldn't actually need these lines (it will just find the default priors).

However, my personal feeling is that we should do out best to keep the default priors small to avoid the code doing things that people don't expect. We could do this for the MR by removing the additions to the default priors, and just placing them in the example scripts, e.g.

priors['Q'] = tupak.prior.Uniform(2, 50, 'Q')
priors['frequency'] = tupak.prior.Uniform(30, 2000, 'frequency')

If things have obvious defaults, then that is okay. But I do worry that with something like 'frequency' there is a good chance that different models will have such an argument, but the default prior won't make any sense. This could lead to unexpected behaviour if people don't know that under the hood we set default priors up!

What do you think about just placing all the priors in the scripts themselves and removing the default priors? Or, just leave them in but make the names as descriptive as possible?

Hi Greg,

Yeah I am happy with that. Do you want me to change it and then push again?

If you could that would be great :)

added 1 commit

• de2b3831 - changes to SN and SG examples

Compare with previous version

added 109 commits

• de2b3831...04f2bf5f - 105 commits from branch master
• f8095131 - Reformatting only
• 7ab773fa - Update priors on coefficients
• 5053fbc1 - Merge branch 'master' into burst
• 9c82e28c - Remove injection_parameters passing

Compare with previous version

Hi @jade.powell

I've just pushed some minor changes and merged master into this (making it easier to merge this back into master)

Summary of changes

1. Changed the priors on the coefficients to -1, 1. This was in a previous version and, when I changed it in the example, the sampler managed to converge within about 10 minutes (with just 100 live points, we may need to increase that if you feel its not converged properly).

2. Removed the injection_parameters passing in. The inferred parameters are not the same as the injected parameters so this was breaking any attempt to make a corner plot.

3. Minor code syntax clean up

Here if the corner plot it now produces, does this make sense?

added 1 commit

• 47cec64a - Add time prior and increase number of live points

Compare with previous version

enabled an automatic merge when the pipeline for 47cec64a succeeds

merged

mentioned in commit 76e321c2