simonsobs / LAT_MFLike

Multifrequency Likelihood for SO Large Aperture Telescope
https://lat-mflike.readthedocs.io
Other
4 stars 12 forks source link

get_foreground_model refactor #83

Closed cmbant closed 2 months ago

cmbant commented 4 months ago

Last one for the moment.. just tries to simplify the code so that foregrounds are used directly as arrays from model dictionary, rather than introducing new large temporary fg_dict structure, most of which was unused.

Now _get_foreground_model is not used internally; I assume this is intended for plotting etc, in which case probably best renamed to get_foreground_model as public facing.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 71.79487% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@660e0fa). Learn more about missing BASE report.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/83/graphs/tree.svg?width=650&height=150&src=pr&token=qrrVcbNCs5&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs)](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/83?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs) ```diff @@ Coverage Diff @@ ## master #83 +/- ## ========================================= Coverage ? 83.33% ========================================= Files ? 3 Lines ? 396 Branches ? 0 ========================================= Hits ? 330 Misses ? 66 Partials ? 0 ``` | [Files](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/83?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs) | Coverage Δ | | |---|---|---| | [mflike/mflike.py](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/83?src=pr&el=tree&filepath=mflike%2Fmflike.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs#diff-bWZsaWtlL21mbGlrZS5weQ==) | `90.40% <100.00%> (ø)` | | | [mflike/theoryforge.py](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/83?src=pr&el=tree&filepath=mflike%2Ftheoryforge.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs#diff-bWZsaWtlL3RoZW9yeWZvcmdlLnB5) | `76.43% <60.71%> (ø)` | |
xgarrido commented 4 months ago

It loooks sensible especially the add of sum_all/term variables. The _get_foreground_model is not only used for plotting purpose since pspipe relies on it to get the foreground model the same way the likelihood does. Since the function declaration does not change, I think we are safe (let's see what @thibautlouis and @adrien-laposta think about it).

cmbant commented 4 months ago

Btw, this currently seems to do nothing: https://github.com/simonsobs/LAT_MFLike/blob/master/mflike/theoryforge.py#L626

sgiardie commented 4 months ago

Btw, this currently seems to do nothing: https://github.com/simonsobs/LAT_MFLike/blob/master/mflike/theoryforge.py#L626

Yes that's true, it has not been fully developed due to the absence of a systematic template to use. I have actually modified that in a branch for some ACT tests, but I doubt it will end up in the master