TopHatWindow (mis-)use

[xgarrido]

In the context of Simons Observatory, we would like to use the TopHatWindow to only store the bin edges. So far, we have been using with success the BandpowerWindow and we store them with the following idiom

sacc_obj.add_ell_cl(..., window=bp_window)

We can't do that with the TopHatWindow and we are getting errors due to this line

sacc/sacc/sacc.py

Line 1203 in 767686a

if isinstance(window, BandpowerWindow):

that do not duplicate windows as it is done with BandpowerWindow. I guess we are wrong in the way to use/store the TopHatWindow. It may be related to the fact that it is not clear what is exactly the content and attributes of the TopHatWindow class. The docstring says the min/max class attributes should be int/float scalars but the unit test

sacc/test/test_sacc2.py

Line 510 in 767686a

def test_tophat_window():

actually plays fine with numpy arrays. In my mind, the min/max attributes should be the bin edges and then numpy arrays.

If we overpass the

sacc/sacc/sacc.py

Line 1203 in 767686a

if isinstance(window, BandpowerWindow):

issue then we get errors when retrieving the window via the get_bandpower_windows function due this line

sacc/sacc/sacc.py

Line 973 in 767686a

if not isinstance(ws, BandpowerWindow):

. Here again, I think we miss the point on how to use this window.

If we treat the TopHatWindow the same way as BandpowerWindow, I can propose a PR with little modification (mainly I write a get_section method for TopHatWindow to extract a subset of bin edges given the selected indices) but I would like to hear from sacc experts or TopHatWindow users how to properly use this window.

Also related to #72

[joezuntz]

Hi @xgarrido. The original intention for TopHatWindow was that it was treated as the window for a single data point, rather than a set - that's why a float / int was the documented usage. So you would make a different window object for each data range (you can re-use the same one between multiple data points that share the same ell range, e.g. one for TT and one for EE).

But that may not be the best design - if you have thoughts about a better one that would be really useful.

[xgarrido]

The basic idea (and what we actually need) is to make the TopHatWindow objects behave like BandpowerWindow especially wrt to multipole selection. I think it may be easier to understand what we are talking about with code and diffs so I just made, within my forked repository, a PR (xgarrido#1) with the changes we would need. I'm not 100% sure it will be backward compatible but I don't really have a practical example (the unit tests are still passing).

[xgarrido]

Hi @joezuntz, do you have any feedbacks ? I think it will not break the API and for us, it will decrease a lot the storage footprint. We may do it differently (storing the path to the binning file in the metadata bank, for instance) but we will lose the multipole selection ability.

[joezuntz]

Hi @xgarrido

I don't quite understand why you don't want to use the TopHatWindow as is - if multiple bins share the same window object it won't be saved multiple times, if that's what you're worried about.

[xgarrido]

Sorry if we misunderstood how to use the TopHatWindow. For instance, if I follow this notebook which stores a list of TopHatWindow windows, how can I retrieve the binning array given multipole selection ? What we currently use is the following function (with names refering to the notebook tutorial and with no multipole selection here):

lb, cb, ind = s.get_ell_cl(galaxy_density_cl, "bin_0", "bin_0", return_ind=True)
ws = s.get_bandpower_windows(ind)

This raises an issue not because we are not using a BandpowerWindow (https://github.com/LSSTDESC/sacc/blob/master/sacc/sacc.py#L973) but because we do not have a unique list of windows in memory (https://github.com/LSSTDESC/sacc/blob/master/sacc/sacc.py#L966) which is a bit misleading.

I understand if you tell us "you must not use the get_bandpower_windows with anything other than BandpowerWindow objects", fair enough, but then how can we retrieve the binning information with the ability of selecting multipole ranges ?

What we propose is to extend this feature to not only BandpowerWindow but also to Log/TopHatWindow by storing/retrieving the binning information the same way as Bbl.

[joezuntz]

Hi @xgarrido

Right now get_bandpower_window is specific to the BandpowerWindow type, but you're right, it would be nice to extend that to something else, do have a go!

Right now you could do this to get the :

windows = s._get_tags_by_index(["window"], indices)

or

window = s.data[i].get_tag('window')

I agree the two are not that clean.

[xgarrido]

Hi @joezuntz

I have tried the different methods you suggest to retrieve windows. It actually works for both window type i.e. BandpowerWindow and (Log)TopHatWindow. Nevertheless, we then have to recompute the whole window given the selected indices. This is already done in the get_section method of BandpowerWindow class and we will have to do it for (Log)TopHatWindow objects. That's why we propose to overload the get_section method for (Log)TopHatWindow to build the whole binning information given selected indices.

Here again, we can do it on our side i.e in SO LAT likelihood but we think, it is worth sharing this work at the sacc top level. So I have created a PR #83 that you can comment/amend or discard if you think it's not worth it.