Center pointings

[mj-gomes]

This PR implements the request in #504.

I implemented the centering by doing ang2pix and pix2ang in sequence, I believe this is the way to do it, as healpix will recompute the angles based on the center of the pixels, but tell me if there is a better way to do this.

p.s. Right now the center_pointings method should always be called after precompute_pointings. I was thinking that maybe we should add the option to center in the other cases, since there is the option to scan a map without precomputing the pointings, and in that case the pointings are computed on the fly and, as of now, not centered. What do you think? Should we add a flag to scan_map_in_observations (and the low level methods that it calls like fill_tod and get_pointings) that will allow to center the pointings also when they are computed on the fly?

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
litebird_sim
io.py
madam.py
mpi.py
observations.py
pointings_in_obs.py
scan_map.py
simulations.py
litebird_sim/mapmaking
binner.py
brahmap_gls.py
destriper.py
Project Total

This report was generated by python-coverage-comment-action

[ziotom78]

Hi Miguel. Thanks for being so quick! Luca, Marta, and I had a chat about this, and we think it would be better to support an “on-the-fly” computation of the centered pointings. This has the advantage of making it super-easy to debug cases like Marta's; one can just call get_pointings(center=True) to get the centered coordinates for saving or plotting without affecting the rest of the simulation. Usually, we don't want to force centering globally.

It should be quite easy to implement, as LBS already has a private function, _get_centered_pointings() (in pointings_in_obs.py), which takes a pointing matrix and returns the “re-centered” version. The code is already tested and could be included in the Observation.get_pointings() method (in observations.py). It would be triggered if get_pointings() receives a new Boolean parameter (center) set to True.

What do you think? Does this approach sound reasonable to you?

[mj-gomes]

It should be quite easy to implement, as LBS already has a private function, _get_centered_pointings() (in pointings_in_obs.py), which takes a pointing matrix and returns the “re-centered” version. The code is already tested and could be included in the Observation.get_pointings() method (in observations.py). It would be triggered if get_pointings() receives a new Boolean parameter (center) set to True.

What do you think? Does this approach sound reasonable to you?

Hi Maurizio, the only problem I see is that the Observation class does not have information about the nside, right? In that case, we should also give the nside as an argument to get_pointings() when we call it with center=True.
Is this correct?

[ziotom78]

Hi Maurizio, the only problem I see is that the Observation class does not have information about the nside, right? In that case, we should also give the nside as an argument to get_pointings() when we call it with center=True.
Is this correct?

Ah yes, you’re right, it’s probably better to implement a new parameter nside_centering: int | None = None, which defaults to None (no centering, which is the current behavior).

[mj-gomes]

Ok, I ended up removing the method I was using because _get_centered_pointings() does in fact the same thing. The way to get the pointings centered in simulation level is now through precompute_pointings() instead of having a separate function, so we simply do:
sim.precompute_pointings(center=True, nside_centering=nside)

And we can now do it directly in obs.get_pointings() as asked, in the same way as above, i.e.:
obs.get_pointings(center=True, nside_centering=nside)

Note that I implemented the centering explicitly in obs.get_pointings since calling the other method resulted in a circular import (importing a method from pointing_in_obs.py in observations.py). The other option would be to implement the centering in the PointingProvider class in pointings.py and center the pointings for each detector, but doing it only once for the entire pointing matrix is, I suppose, probably more efficient.

Does this look ok?

[ziotom78]

Hi Miguel, thanks! My suggestion to use int | None for nside_centering was that this makes center redundant: you already know if you want centering or not if nside_centering is None or an integer. So, center can be removed.

[mj-gomes]

Hi Miguel, thanks! My suggestion to use int | None for nside_centering was that this makes center redundant: you already know if you want centering or not if nside_centering is None or an integer. So, center can be removed.

Ah, of course. It's done now.

[ziotom78]

Note that I implemented the centering explicitly in obs.get_pointings since calling the other method resulted in a circular import (importing a method from pointing_in_obs.py in observations.py). The other option would be to implement the centering in the PointingProvider class in pointings.py and center the pointings for each detector, but doing it only once for the entire pointing matrix is, I suppose, probably more efficient.

Ok. My only concern now is that centering the pointings in precompute_pointings only makes sense when you want the pointings to be always centered from the beginning of a simulation till the end, but this is probably never the case. My original idea was to only have the on-the-fly option to compute centered pointings in Observation.get_pointings() and not to touch precompute_pointings(). But I would like to hear @paganol’s opinion too.

[paganol]

Hi there, I agree with @ziotom78 here. Ideally, in my mind, the pointing should be always kept in the original form, i.e. "True" and in Ecliptic coordinates. Then, when you need a different flavour, you call the function (e.g. Observation.get_pointings()) that returns the pointing in the form you want. If we have time we could briefly chat about it tomorrow morining.

[mj-gomes]

As decided in the last meeting, I removed the option to center the pointings in precompute_pointings and in the Simulation level, which would also change the pointings in all observations' pointing matrices.

I think this is ready to merge.

[ziotom78]

Hi Miguel, perhaps these changes are spurious? They do not seem to be related with the issue of pointing centering.

[ziotom78]

Sorry, I was referring to some of them, of course. Apart from the usual formatting fixes (which are ok to include here), there are tmp_path instances that are no longer used and reports are instead written in the current directory.

[mj-gomes]

The last function added is supposed to test the get_pointings by setting nside_centering to some value. But the changes in the base paths were indeed something I had to do to run the test locally faster without going through pytest, that I forgot to remove in the end, I'll change that in a second.

[ziotom78]

Thanks for your work, @mj-gomes , it looks ok to me!