cta-observatory/pyirf

Code duplication between background_2d and estimate_background?

kosack opened this issue · 2 comments

kosack commented

Both sensitivity.estimate_background() and irf.background_2d() do essentially the same thing: they compute the background rate, but output very different structures: one a set of histograms in table format, and the other a bare 2D array. The only real difference is that one computes it in all offsets annuli at once, and the other in a single annulus. Why are these in different modules? Could we merge them into one common function?

maxnoe commented

This was intentional, one is the function to compute the background IRF in GADF format (the one in pyirf.irf), the other is to immitate what EventDisplay is doing in the senstivity estimation.

So while I agree that these functions share similarities, they are for entirely different scopes, so I'd be wary to let them share code.

kosack commented

I don't really see how those are so different: you are mixing computation and format, I think.
And doesn't that mean that the IRFs you produce and sensitivities you produce may not match in the case a bug is fixed in one place but not another?

I would have the functionality clearly separated:

  1. functions to compute IRFs
  2. functions to compute sensitivity from existing IRFs
  3. functions to read and write IRFs to GADF format (or any other format)
  4. functions to optimize cuts (which requires a loop on 1 and 2 in the case of optimizing sensitivity)