Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code duplication between background_2d and estimate_background? #265

Open
kosack opened this issue Sep 28, 2023 · 2 comments
Open

Code duplication between background_2d and estimate_background? #265

kosack opened this issue Sep 28, 2023 · 2 comments

Comments

@kosack
Copy link

kosack commented Sep 28, 2023

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
Copy link
Member

maxnoe commented Sep 28, 2023

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
Copy link
Author

kosack commented Sep 29, 2023

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants