litebird / litebird_sim

Simulation tools for LiteBIRD
GNU General Public License v3.0
18 stars 13 forks source link

Add TOD interpolation #233

Closed marcobortolami closed 1 year ago

marcobortolami commented 1 year ago

This PR adds the possibility to choose the TOD interpolation when scanning the input map.

marcobortolami commented 1 year ago

It would be nice to have the possibility to interpolate the TODs before using the bolometric equation here, for example by using this function of healpy. I added a flag interp for choosing the type of interpolation in scan_map_in_observations. The flag is an empty string for no interpolation (the default value and the case implemented before this PR) and it is "linear" for a linear interpolation with the healpy function. In the future, other kinds of interpolation could be added. I used the healpy function, but maybe the healpix one is better. What do you think?

marcobortolami commented 1 year ago

I attach here a comparison of the TODs scanning without and with the interpolation interpolation comparison and a zoom interpolation comparison_zoom

mreineck commented 1 year ago

I hope that in the future it will be possible to get "perfectly" interpolated values for a given sky and beam, either via totalconvolver (without HWP) or MuellerConvolver (with HWP). Unfortunately, at least the MuellerConvolver will still take a few weeks to finalize ...

marcobortolami commented 1 year ago

Thank you @paganol, the code is more clean now:) @mreineck yes, we should bring up this point after Mueller Convolver is finished:) Two questions:

  1. Are we happy with the use of healpy? So, in general, with the linear interpolation section?
  2. Do you think we should add/use None instead of an empty string for the default value of interpolation?
ziotom78 commented 1 year ago

Hi @marcobortolami , thanks for the PR!

Are we happy with the use of healpy? So, in general, with the linear interpolation section?

I checked if the function is implemented in ducc, but it seems that Healpix_Base::get_interp is not exported in Python. @mreineck , is this right? (At the moment, litebird_sim depends on ducc 0.25, but it seems that the function is still not available in ducc 0.30.)

Do you think we should add/use None instead of an empty string for the default value of interpolation?

I think that scan_map(…, interpolation="none") is clearer, so I added this to the repository (but the empty string still works as intended).

I have added a mock test, just to check that the new code path compiles and executes; it does not check the correctness the result, but I do not think it's a big deal, as we assume that the interpolation routine implemented by healpy/ducc is already well tested.

mreineck commented 1 year ago

It's true, the function is not exposed on the interface yet, but I can change this fairly easily if needed. On the other hand, it's the same interpolation technique that's also used by healpy, so there is no urgent reason to change, as long as healpy is also needed for other things.

ziotom78 commented 1 year ago

It's true, the function is not exposed on the interface yet, but I can change this fairly easily if needed. On the other hand, it's the same interpolation technique that's also used by healpy, so there is no urgent reason to change, as long as healpy is also needed for other things.

Oh, I see, I assumed that ducc's implementation was more efficient.

It's not that urgent to export that function, we can rely on healpy for the time being. (In the mid/far future however, I would like to base our code only on ducc and completely remove healpy as a dependency.)

mreineck commented 1 year ago

The implementation is pretty much the same, but you have a point: the ducc interface might be faster, and there will be the option of using multithreading. I'll try to add this to the interface soon! If you encounter other required functions, please let me know!

marcobortolami commented 1 year ago

Hi:) just a kind reminder of this PR. Do we use the healpy interface (closing the PR) or try to use the ducc one (waiting for closing the PR)?

mreineck commented 1 year ago

I'd vote for committing the patch as it is, the code works fine after all! I can take care of adjusting the calls once we have the functionality in ducc.

marcobortolami commented 1 year ago

mmmh I don't know why but when I pulled the GitHub changes to my local repo it automatically did a commit. Plus, changing the order of the changelog, so the issues and PR are in ascending order, gives a conflict. Sorry for that, do you know how should I solve this? I don't want to cause more troubles ;)

ziotom78 commented 1 year ago

Weird, I sorted again the entries in CHANGELOG, now it should be ok. Thank you!

paganol commented 1 year ago

We should:

marcobortolami commented 1 year ago

Hi @paganol, I think in #244 you worked on part of or all the items on your checklist, right? What can we mark as completed?