Closed ziotom78 closed 1 year ago
From my point of view, this PR is ready to be reviewed. It introduces a number of breaking changes:
make_bin_map
has changed: the inverse of the N_obs matrix is always returned, and the result is no longer a map or a tuple (depending on the input parameters) but instead a BinnerResult
object. This matches the way make_destriped_map
and destripe_with_toast2
work.destripe_with_toast2
and save_simulation_for_madam
has been renamed ExternalDestriperParameters
The source files have been organized in a more coherent way: the binner and the destriper are kept in the sub-module litebird.mapmaking
, which is however imported in litebird
. Thus, the following still works:
from litebird_sim import make_bin_map
but now one can write
from litebird_sim.mapmaking import make_bin_map
litebird_sim/toast_destriper
Finally, the problems I reported about a strange increase of power at high multipoles has been solved. It was due to an incorrect iteration over the detectors, such that all the detectors shared the same values for the baselines. With this bug fixed, the results of the new destriper agree quite well with TOAST2:
After a call with @paganol , I have implemented the following additional changes:
make_bin_map
into make_binned_map
, as this matches make_destriped_map
NSIDE
is no longer passed to make_destriped_map
through the class DestriperParameters
; instead, it is the very first argument, and this is the case for make_binned_map
too (previously, it was the 2nd argument). In this way, the call to the two functions looks more similar.NobsMatrix
implements the method get_invnpp()
. It returns the inverse M⁻¹ of the Nobs matrix Ⅿ, which is useful to estimate the error per pixel.Finally, I checked that when make_destriped_map
is called while setting DestriperParameters.samples_per_baseline
to None
, it returns the very same map as the one produced by make_binned_map
. (I only tested this in one case, but it was quite complex and I trust the result.) The same applies to the field BinnerResult.invnpp
, which matches DestriperResult.nobs_matrix_cholesky.get_invnpp()
.
This means that in the future we might think about deprecating the current implementation for make_binned_map
and rewrite it as a very tiny wrapper around make_destriped_map
.
What's left is to check that the amount of memory used by make_destriped_map
matches the expectations, then I believe we can merge this PR and release version 0.11.0.
Hi:) just to let you know, I'm also reviewing the code:) I don't know if it can be seen, I'm using the GitHub review tool for the first time, I'm waiting to check all the files before submitting it, so my review is still "Pending". Anyway, I fully agree to the changes that came out after your call with Luca:)
Thanks for your quick fixes:) for me this is ready to be merged!
Hi there. Thanks a lot!! You can merge for me, as well.
Thank you all, especially @marcobortolami : your review was comprehensive and perfect!
This PR implements a very simple destriper for the framework.
Here is the main highlights:
remove_baselines_from_tod
, can clean the TODs inObservation
objects for the baselinesSimulation
class and adds information to the reportmake_bin_map
Observation
object (example:sky_signal
,dipole
,noise
…), with no additional memory requiredPain points:
Stuff to do:
make_bin_map
and the TOAST2 destriperremove_baselines_from_tod