search

simonsobs / LAT_MFLike

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

Simpler calibration #82

Closed cmbant closed 2 months ago

cmbant commented 4 months ago

I found the calibration code quite hard to understand, esp. why per likelihood it was instantiating a "residual" class in a systematics library. This just does it directly (assuming I correctly understood the original..). Rotation_alm is then not needed, and should also be faster/many fewer temporary memory allocations.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 92.59259% with 2 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/82/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/82?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs) ```diff @@ Coverage Diff @@ ## master #82 +/- ## ========================================= Coverage ? 87.34% ========================================= Files ? 3 Lines ? 395 Branches ? 0 ========================================= Hits ? 345 Misses ? 50 Partials ? 0 ``` | [Files](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/82?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/82?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/82?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) | `84.73% <87.50%> (ø)` | |
sgiardie commented 4 months ago

Hi @cmbant, yes your code seems to reproduce what we have in the master. The current structure for effects like calibrations, polarization angles and systematic template is to call the corresponding classes in syslibrary, the library for systematic effects, as developed by @mgerbino. I guess we can have a discussion about implementing these functions directly in mflike if that is more convenient from a computational point of view (@mgerbino, @paganol, @erminiacalabrese, @xgarrido)

cmbant commented 4 months ago

Thanks for checking. The others probably make more sense to have separately. This was particularly odd a) because calibrations have a trivial structure and you could argue about whether it is a systematic (and certainly isn't a residual), and b) the introspection in set_defaults() etc was being done once per call rather than at initialization, i.e. it didn't really seem to fit the structure.

(Is there an explanation somewhere of why the model.py introspections for changing defaults are needed at all? Anything like this makes it much harder for IDE tools to navigate the code/automatically check for bugs, as well as harder to understand)