Adding counter to likelihood

added 1 commit

1b13176d - pep8

resolved all discussions

added 1 commit

beef3589 - removing redundant line calling super

Hi @rhys.green, can you merge master into this branch, we (mostly Greg) fixed a bunch of stuff in the CI today, so it may just work.

added 3 commits

beef3589...8c03c68f - 2 commits from branch lscsoft:master
c1d4b468 - Merge branch 'master' of https://git.ligo.org/lscsoft/bilby into adding_counter_to_likelihood

Compare with previous version

Hi Rhys, nice idea. I can see the motivation for this, but I just want to check a few things.

Is not possible to get this in post processing already? I can't directly see how, but it would be cleaner rather than doing it while sampling.
There is a built-in Counter object in python, it would be really good to use this as this is the sort of thing which someone sees and wants to add "prior_evaluation_counter", "other_thing_counter" etc etc. The Counter means there is only one object and there are explicit names for what it is counting.
There are a few things I'm a little concerned about in the implementation (see the review comments) - if we have to hack things into weird places then okay, but it would be best if we don't.
There are a few places where you have added an extra line which isn't relevant to this MR. Can you remove these? It is petty I know, but it makes things much simpler if the MR doesn't include various minor additions/deletions.

Hi Greg, thanks for looking at this !

that was my first thought too but I couldn't see any obvious ways to do it. I tried doing this with something like profiling but that seemed messier than this way ? If you have any suggestions I'd be happy to change the implementation
I was not aware of the counter object so yea it makes sense to use that
I agree, (see replies in review above)
Sure I don't think those were actually supposed to be in this MR

to follow up on point 2 I'm not sure if the counter object is the best thing to use, it looks like it's not really designed to count function calls. I think the manual counter is probably cleaner If we want to make it more general, Maybe specify self.likelihood_counts rather than self.count? you could then keep the add_count function but have other self.something_else_counts like you mentioned ?

resolved all discussions

added 1 commit

faffbc60 - replacing self.count with result.num counter, this makes the code cleaner as...

Compare with previous version

added 1 commit

edec6e8a - adding changes to all samplers, re-adding line after init

Compare with previous version

added 35 commits

edec6e8a...4d2d5d36 - 34 commits from branch lscsoft:master
bdb9b9bb - pulling latest and fixing merge conflicts

Compare with previous version

added 1 commit

3dfdc69a - pep 8;

Compare with previous version

@john-veitch @colm.talbot @gregory.ashton @christopher-berry this works fine for each of the samplers apart from cpnest. The counter works for cpnest i.e. you can force it to print whilst sampling and the counter goes up but I think potentially at some point after sampling the result object is called again and the num_likelihood_evals is set to None. Anyone have any idea why this is happening/a fix ?

added 1 commit

03c0d57e - removing unrelated change

Compare with previous version

Hi @rhys.green. What do you mean the result object is "called again"? CPNest can't access anything in its Bilby control class so it can't be over-writing it.

I'm guessing this is because CPNest runs its samplers in separate processes. That means that the wrapper each sampler sees is a copy of the parent process's object as it was before you call cpnest.run(). When the samplers finish their copies of the object are destroyed, and the likelihood counts along with them. I don't see any way of passing this back to Bilby without inter-process communication, so how do the other multi-threaded samplers do it?

Probably the simplest thing to do is to add this functionality to CPNest. I'm already thinking of overhauling the IPC to fix the checkpointing bug, and that'll make it much simpler to track this sort of information.

Alternatively, and as a quick sanity check, you could declare your counter as a multiprocessing.Value object so that all processes can see it. I wouldn't recommend using that for benchmarking though as it'll slow things down to some extent.

added 3 commits

8f368893 - reverting to using add_count function
323071a8 - reverting to using add_count function
be2df0af - reverting to using add_count function

Compare with previous version

added 1 commit

a368d15b - typo

Compare with previous version

Hi @john-veitch what I meant by that is if I print the self.likelihood_count inside of the cpnest model log_likelihood (line 70 of cpnest.py) the numbers go up, so it must be getting that count information from the base sampler? After it has finished sampling the count goes back to zero, I didn't realise the thread copies of the result object were destroyed so this potentially explains it.

Ideally I'd like to be able to return the likelihood count information to the "parent" result objects in a similar way that the samples are but I'm not sure how to do this through bilby.

I'll try it with multi-processing value object as I suppose using that would still give a fair score in terms of samples per likelihood evaluation.

Also in the logging information when a cpnest run finishes, a quantity called input samles is returned. What is this ?

added 2 commits

3f99d311 - changing implementation to allow for multiprocessing, this is much nicer than before and works
7ea67c04 - changing implementation to allow for multiprocessing, this is much nicer than before and works

Compare with previous version

Ok so I think this now works, it's using the multiprocessing value object. I haven't noticed any significant slowdown from that but I haven't checked quantitatively. I think this implementation is okay and is potentially ready to merge if people want to include this ? @john-veitch @christopher-berry @gregory.ashton @colm.talbot what do you think ? Obviously I'm happy to change things if you guys want

Sounds good to me. So long as the feature can be turned on/off we shouldn't worry too much about a slow down in wall-time.

Is there any unit testing that we need?

added 1 commit

6d5dcb78 - typo - only assigned num_likelihood_evals in test section

Compare with previous version

@rhys.green,

if I print the self.likelihood_count inside of the cpnest model log_likelihood (line 70 of cpnest.py) the numbers go up, so it must be getting that count information from the base sampler? After it has finished sampling the count goes back to zero, I didn't realise the thread copies of the result object were destroyed so this potentially explains it.

Just to be clear, you are confusing multiple objects that are held by different processes. When you add a print statement, it is executed in one of the parallel processes, so it is printing the counter that belongs to that process. The version that is held by the main process does not get printed or incremented unless the likelihood is called from that process, which it isn't. So it's not "going back" to zero, it always was.

Using a multiprocessing.Value will allow you to work around this because then all processes see the same counter. It should work for any code that's based on the multiprocessing module, but that is not the only possibility. However since you just want to make progress and it doesn't produce a lot of overhead then it's probably ok.