maho3 / ltu-ili

Robust ML in Astro
https://ltu-ili.readthedocs.io/en/latest/
46 stars 9 forks source link

New classes for SampleBasedMetric, custom signatures for plots and summarizer dependence. Also adds automated unit tests #83

Closed CompiledAtBirth closed 1 year ago

CompiledAtBirth commented 1 year ago

Hello, below is the list of changes in this PR:

Please share your thoughts, especially about the ensemble_mode feature which I took the initiative to implement to deal with the case where we want to compare different models (MAF, MDN...) in the same sbi runner instance.

maho3 commented 1 year ago

Overall, this PR is huge. Really great work. This cleans up many of the details that we needed fixed a while ago. Thanks Nicolas

The ensemble_mode thing is a really good idea. That will be really useful for checking multiple architectures at once. People may just end up using ensemble_mode=True most of the time, but the option will be very useful for trial-and-error.

I need to do a thorough review this week of all these changes. Here are some checklist for myself:

CompiledAtBirth commented 1 year ago

Thank you Matt for the comments and the checklist. @DeaglanBartlett, since issue #68 involves a lot of work for the test suite, do you want to link the issue here as well before thinking about merging? I hope the fact that the PosteriorCoverage class replaces the rank stats and TARP metrics, among other things, is not a pain to rewrite existing validation scripts.

DeaglanBartlett commented 1 year ago

@CompiledAtBirth yes it makes sense to wait for this PR to be approved before making a PR for #68 so that the changes made here can be added to the test suite.

CompiledAtBirth commented 1 year ago

Thank you Deaglan for all the changes. I just added the possibility to use existing sims at first round for multiround inference (for working example 2).

maho3 commented 1 year ago

I think this PR is nearly good to go. I've gone around, tested things, and cleaned up code without changing much functionality. I've also rerun the tutorial.ipynb notebook and updated the documentation. Also, I've automatically added '"_"''s to every strsave and signature, so we don't need to add trailing 's in the config files.

However, there's two changes I'd like to recommend, but first want @CompiledAtBirth 's approval on before making them:

1) I think the meaning of str_save is a little obscure at first glance. I would recommend changing it to just name or something.

2) Instead of wrapping the NeuralPosteriorEnsemble in another layer of abstraction as in the PosteriorEnsemble class, I would recommend that we just directly assign signatures as an attribute of sbi's NeuralPosteriorEnsemble.

For example, in its usage for saving in this line, we can just do something like:

posterior = NeuralPosteriorEnsemble(
    posteriors=posteriors,
    weights=val_logprob
)
...
posterior.signatures = signatures

Then, when we pickle NeuralPosteriorEnsemble later, signatures should be saved with it.

This way, we don't have to do the awkward thing of e.g. posterior_ensemble.ensemble.sample to access the useful NeuralPosteriorEnsemble functions, as in the jupyter tutorial. This will also retain consistency with the pydelfi interface.

Just some recommendations, but I'm curious what you think. After these are addressed and changes are propogated through the code (including rerunning tutorial), I think I'm ready to approve the PR.

CompiledAtBirth commented 1 year ago

@maho3 super, thank you for the thoughtful review and cleaning. I agree on both points which make usage and future code modifications more straightforward. Special mention to 2), I did not know that one can add attributes dynamically to a class instance in Python :)