ASAD method for polarization

[eneights]

This includes a class for polarization fitting using the azimuthal scattering angle distribution (ASAD) method, and an example notebook which fits the polarization of a GRB. It's currently able to handle a point source which is stationary with respect to the detector, and a stationary spacecraft orientation.

[codecov]

Codecov Report

Attention: Patch coverage is 78.28054% with 96 lines in your changes missing coverage. Please review.

Project coverage is 74.46%. Comparing base (99fdff7) to head (c6c3ccd).
Report is 13 commits behind head on develop.

Files with missing lines	Patch %	Lines
cosipy/response/FullDetectorResponse.py	2.53%	77 Missing ⚠️
cosipy/polarization/conventions.py	88.07%	13 Missing ⚠️
cosipy/polarization/polarization_asad.py	97.61%	5 Missing ⚠️
cosipy/polarization/polarization_angle.py	97.56%	1 Missing ⚠️

Files with missing lines	Coverage Δ
cosipy/polarization/__init__.py	100.00% <100.00%> (ø)
cosipy/polarization/polarization_angle.py	97.56% <97.56%> (ø)
cosipy/polarization/polarization_asad.py	97.61% <97.61%> (ø)
cosipy/polarization/conventions.py	88.07% <88.07%> (ø)
cosipy/response/FullDetectorResponse.py	35.83% <2.53%> (-2.92%)	⬇️

... and 1 file with indirect coverage changes

[israelmcmc]

Thanks, @eneights! I haven't had time to review it yet, but I just want to add a comment to remind us to merge #232 first (a dependency to this PR)

[israelmcmc]

I haven't had time to review it yet, but I just want to add a comment to remind us to merge #232 first (a dependency to this PR)

[nmik]

I tested the jupyter-notebook and was able to run it through the end.

[israelmcmc]

Closing and reopening this PR to update the list of files changed after merging #232

[israelmcmc]

Hi @eneights. Thanks for working on this. I finally went through all of your code, sorry for taking so long. In summary, this is working fine, but I do want to request a couple of changes for FullDetectorResponse before merging.

• The polarization flag passed by the user is not really needed, since we can get that information from the file itself
• You added large if blocks based on whether the response had polarization information or not, however, these are for the most part duplicated code. This adds to the same problem with the sparse flag, creating a big piece of code hard to maintain.

I created a PR to your PR implementing the changes above. Please check that it still works as you intended, and if so, once merged with your branch we can merge this PR.

I do have other comments for polarization_asad.py. However, since these are self-contained and I want us to focus on the polarization analysis using 3ML, I don't think they need to be addressed before merging this PR. Here they are:

• About calculate_uncertainties. The ASAD method only really works in the Gaussian regime. I think this can be replaced by a simple sqrt(counts) (in-place when needed). Or, if you decide to keep it, it might be better to hide it as a local utility function (not in initi). Since it is not of general applicability, keeping it local saves you from having to maintain it.
• It would be better to store your ASADs as histpy Histogram objects, instead of the dicts. That way you have access to all the methods (e.g. fill(), binary operation -, plot(), etc) that should make your code easier to read and maintain. In addition, related to the point aboe, you can use histpy Histogram the bin_error to get the uncertainties.
• You are computing the error for the ASAD prediction that come from the response the same way as you do for data (sqrt(counts)). This is not really correct. The errors for the response come from the triggered counts in simulations, not from the predicted counts after convolutions. We don't currently keep track of these, but since we use many many simulated events, these should be way smaller than what you are estimating. It might be better to just exclude these errors from the error propagation and just keep the errors for the data.
• I would prefer if calculate_azimuthal_scattering_angle becomes part of the PolarizationAngle class since this is of general use, not just for the ASAD method.
• Various functions take inputs that could be instead computed internally by the class and cache, instead of the users keeping track of them. Even more, I think that it would be possible for the user to initialize the PolarizationASAD object and inmediately go to fit() to get the results.
• I know this was just a proof of principle, but many parts of your code can be optimized a lot. It would be good to run a profiler. Some of the things I noticed 1) sometimes you use project just to get the number of bins 2) use for loops for operations that can be parallelized 3) Repeat operation within a loop that could be cached
• The fit to the constant() function can be replace by just np.mean(), doesn't it?
• If not intuitive when function that are meant to perform a computation also generate plots by default.
• We should be using logging instead of print statements.

We could open an issue with these for future reference if you don't plan to continue working on the ASAD method for now.

[israelmcmc]

I think the tests failing are related to the new ori format after this PR was created:
#263

I added dummy orbital information to your test ori file in my PR to you PR, hopefully that should solve it.

[eneights]

Thanks @israelmcmc! Your changes to FullDetectorResponse look good to me. And I agree with all of your feedback; I'll make those changes when I start working on this. I opened issue #275 to keep track of these.