Normalize Acceptance Rate Plot by ESS #345
Conversation
There was a problem hiding this comment.
Hi, that looks (except one thing) really nice. Could you also add a test case to https://github.com/ICB-DCM/pyABC/blob/master/test/visualization/test_visualization.py#L53? I.e. just run the same code with the new option enabled. I know that this is not a proper unit test, but for visualization that is all we currently do.
| pop_sizes.append(np.array( | ||
| history.get_nr_particles_per_population().values[1:])) | ||
|
|
||
| if normalize_by_ess: |
There was a problem hiding this comment.
I think there is one error here: Not samples should be replaced by ESS, but pop_sizes should be replaced by ESS, t.t. the resulting rations are usually <= 1. So, just move this code block up a bit ;) Sorry, if I miscommunicated that.
There was a problem hiding this comment.
Please also update the docstring accordingly.
|
made all the changes! (Thank you so much for the review :) ) |
There was a problem hiding this comment.
Nice, thanks!
(And in your plot I see that we need to enforce integer values on the x axis when plotting populations. Not sure how to best do that -- needs to be integer, but should not be all integers in case many time points exist. Leaving that for future work :) .)
| @@ -74,7 +74,8 @@ def test_acceptance_rates_trajectory(): | |||
| histories, labels, yscale='log', rotation=76) | |||
| _, ax = plt.subplots() | |||
| pyabc.visualization.plot_acceptance_rates_trajectory( | |||
| histories, labels, yscale='log10', rotation=76, size=(10, 5), ax=ax) | |||
| histories, labels, yscale='log10', rotation=76, size=(10, 5), ax=ax | |||
| normalize_by_ess=True) | |||
There was a problem hiding this comment.
One last thing: Could you once run this with normalize_by_ess=False, and once with True? So as to test both modes.
|
Great! As the tests do not seem to run, I will change base and from there do a PR do develop. |
Tackles issue #202
Example plot (with acceptance rate default and ESS plot)
