Resolve "Make sure marginalized parameters are not sampled"

Closes #271 (closed)

changed milestone to %Future

added Feature-request label

added 4 commits

• cd70b9f1 - Added new Error class
• 3c7f5b1f - Adds logic to check if marginalized parameters are being sampled
• 57dd9e21 - Adds public reference phase and reference time properties, makes reference distance public
• 40948868 - Adds test for check for sampling in marginalized parameters

Compare with previous version

added 1 commit

• ea6b6300 - Only return reference parameter values if likelihood is marginalized

Compare with previous version

added 1 commit

• 6c8dec51 - Fixed PEP8 issues

Compare with previous version

added 1 commit

• 0e01a3fb - Fixed some remaining issues with marginalisation sampling test

Compare with previous version

unmarked as a Work In Progress

I understand the motivation for this MR, but the enumeration of GW things feels very specific for something that happens every time run_sampler is called.

Personally, I'd prefer to not have this feature.

Would it not be easier to just do

if getattr(likelihood, "prior", prior) != prior:
    raise SamplingMarginalisedParameterError()

or something? (I'm not sure that one can actually equate dictionaries like this, but some equivalent). The main point is just: is the prior the same as the likelihood prior?

@gregory.ashton the two priors explicitly should be different. The likelihood keeps the original prior used for the marginalisation but the sampled prior should be different.

I can also fit it into the GravitationalWaveTransient class. I still think this would be a useful feature to have because I can see how people would make this mistake.

If we add a marginalized_parameters attribute to the base Likelihood class. We can then add a logical check like

for key in likelihood.marginalized_parameters:
    if key in priors and not isinstance(priors[key], (float, DeltaFunction)):
        raise SamplingMarginalisedParameterError()

@moritz.huebner is this something you plan to get merged in for this release? If so please tag it as such to avoid it being skipped.

changed milestone to %0.5.9

added 35 commits

• 0e01a3fb...b7915477 - 33 commits from branch master
• 8fa49b49 - Merge remote-tracking branch 'origin/master' into...
• 265834ca - Worked in Colm's suggested solution for checking if marginalized parameters are sampled

Compare with previous version

added 2 commits

• 5aee43f0 - Worked in Colm's suggested solution for checking if marginalized parameters are sampled
• 2fa179a4 - Merge remote-tracking branch...

Compare with previous version

@colm.talbot @gregory.ashton I changed this as it was suggested by Colm and it is now ready to be merged

added 1 commit

• 4c91fee6 - Resolved a PEP8 issue

Compare with previous version

resolved all threads

approved this merge request

approved this merge request

mentioned in commit 3bf09dae

merged