LSSTDESC / firecrown

DESC Cosmology Likelihood Framework
BSD 3-Clause "New" or "Revised" License
29 stars 7 forks source link

Save predictions to sacc #349

Closed tilmantroester closed 7 months ago

tilmantroester commented 9 months ago

This PR addresses #346.

It writes the predicted theory vector into a sacc object, replacing the corresponding data point. By default the method raises an error if the sacc object contains data points that are not being overwritten by the predictions to avoid inconsistency.

Future additions to the functionality would be to draw a sample from the likelihood, instead of using the (zero-noise) theory prediction.

One discussion point is how to store the predictions. At the moment, only TwoPoint does this and uses predicted_statistic_ for this. I've used predicted_statistic_ for now for compatibility with existing code but I wouldn't mind renaming this to theory_vector to match the data_vector.

The test doesn't cover the checking of the prediction against the content of the sacc object yet.

tilmantroester commented 9 months ago

Apart from the missing test and mypy errors, this can be reviewed I think @marcpaterno @vitenti

vitenti commented 8 months ago

@tilmantroester , please, take a look on the modification to see if you agree with the current version.

tilmantroester commented 8 months ago

Thanks for the help! I think adding noise to the prediction should be factored out of the functionality of saving the prediction. That way the functionality can also be used to save predictions+noise during sampling and would address #323.

vitenti commented 8 months ago

@tilmantroester , what about making it optional? Take a look on the last commits.

tilmantroester commented 8 months ago

I'd move make_realization to Likelihood or GaussFamily. Other likelihoods should still be able to save theory predictions, even if the sampling from the likelihood isn't implemented yet. We could add an abstract method Likelihood.sample, with the Gaussian and Student-T likelihoods implementing the specifics (I have the math for the latter).

vitenti commented 8 months ago

I'd move make_realization to Likelihood or GaussFamily. Other likelihoods should still be able to save theory predictions, even if the sampling from the likelihood isn't implemented yet. We could add an abstract method Likelihood.sample, with the Gaussian and Student-T likelihoods implementing the specifics (I have the math for the latter).

I think that we came up with the same solution, I just implemented your suggestion. Please, take a look.

vitenti commented 8 months ago

@tilmantroester I think that it is now complete. Please tell me if you think there is something else to do here, otherwise we will review and merge it during our next Firecrown meeting.

tilmantroester commented 8 months ago

Looks good to me, thanks!

codecov[bot] commented 7 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (b978402) 95.22% compared to head (ef3b08e) 95.42%.

Files Patch % Lines
firecrown/likelihood/gauss_family/gauss_family.py 96.66% 2 Missing :warning:
firecrown/likelihood/likelihood.py 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #349 +/- ## ========================================== + Coverage 95.22% 95.42% +0.20% ========================================== Files 36 36 Lines 2490 2556 +66 ========================================== + Hits 2371 2439 +68 + Misses 119 117 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.