Fix likelihood count for Dynesty

Changes calc_likelihood_count for Dynesty to include the internal counts from the sampler. This fixes the discrepancy between the saved number and the number displayed in the log files as mentioned in #516 (closed).

Any calls to likelihood prior to sampling (e.g. from timing the likelihood here) are still included, however, these calls could be excluded if prefered.

I chose to change calc_likelihood_count over adding something in _generate_result so the number saved to the result file matches the output of this method to avoid confusion if the user called the method elsewhere.

added 3 commits

• 21e07cf7...3c93842c - 2 commits from branch lscsoft:master
• 8c8aef7e - Merge branch 'master' of git.ligo.org:michael.williams/bilby into...

Compare with previous version

Thanks @michael.williams for doing this and sorry for the delay.

I'm concerned that this will lead to double-counting if no multiprocessing is used, I thought that the current method only fails when using multiprocessing?

Hi @colm.talbot, as it stands it doesn't seem to with or without multiprocessing. After another look at it, I've realised it's not because of multiprocessing itself but rather because of how the likelihood function is parsed in general.

As far as I can tell the calls are only counted if log_likelihood from the base_sampler is called (https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/core/sampler/base_sampler.py#L370) but when the likelihood is parsed to dynesty as global variable here the user-defined likelihood object is used instead and this skips the counter.

I haven't tried re-writing it use the likelihood with the counter but I can look into it if you'd prefer.

mentioned in issue #516 (closed)

added 9 commits

• 8c8aef7e...d858a275 - 8 commits from branch lscsoft:master
• 07a26b96 - Merge remote-tracking branch 'origin/master' into fix-dynesty-num-likelihood-evaluations

Compare with previous version

added 63 commits

• 07a26b96...40957da0 - 62 commits from branch lscsoft:master
• 4db4bdeb - Merge remote-tracking branch 'origin/master' into fix-dynesty-num-likelihood-evaluations

Compare with previous version

added 1 commit

• 6a8202ea - Prevent double-counting of likelihood evaluations

Compare with previous version

Hi @colm.talbot, I've had another look at this and my understanding is that counter won't work irrespective of whether multiprocessing is used or not because the likelihood is always wrapped here.

There is one exception which is in _run_test where the likelihood isn't wrapped.

I think the simplest solution would be to rely on dynesty's internal counter and not use the counter in the likelihood. I've changed the merge request to reflect this.

added Likelihood Sampling labels

changed milestone to %1.0.4

approved this merge request

approved this merge request

changed milestone to %1.1.0

approved this merge request

merged

mentioned in commit de381427

mentioned in merge request !928 (merged)