cositools / cosipy

The COSI high-level data analysis tools
Apache License 2.0
3 stars 16 forks source link

Earth occultation in the image deconvolution (ScAtt binning) #182

Closed hiyoneda closed 1 month ago

hiyoneda commented 1 month ago

I added a functionality to consider the earth occultation in the coordinate conversion matrix. The earth occultation can be included in the analysis of the image deconvolution with the scatt binning + local coordinate.

I want to emphasize that it is limited to the case that the satellite always points to the zenith. So, it is valid for the DC2 but not for general situations.

Also, I changed the codes additionally as follows. All of them are minor changes.

I compared the reconstructed spectrum of 3-month Crab data with/without considering the earth occultation effect. The reconstructed flux becomes more consistent with the injected model, especially in the higher energy band where gamma rays can pass through the bottom BGO shield more easily. For example, the difference changes from -26% to 0.3% at the highest energy band.

Screenshot 2024-05-31 at 17 50 50 Screenshot 2024-05-31 at 17 50 57

israelmcmc commented 1 month ago

Hi @hirokiyoneda. Thanks for working on this. I'm OK with all the changes you listed, but I'm on the fence about adding the Earth occultation code itself.

For this workaround, specific to DC2, we don't need to change the code at all, just change the detector response file. Since this new code is not meant to be definitive, and the same can be achieved with a simpler solution --that doesn't involve people getting a new release-- I thought it was cleaner to leave the code as is and save us from more code that needs to be tested and maintained. What do you think?

hiyoneda commented 1 month ago

That's a good point. I wanted to merge it into the development branch because:

But I agree that it is cleaner to keep it as it is.

One concern is that I made minor changes following the earth occultation function. Actually, my current other works depend on them. I am happy if I can merge them into the develop branch. Should I do PR including these minor changes but without the earth occultation functions? Is it a good way to remove the commit regarding with the earth occultation, probably using git revert?

israelmcmc commented 1 month ago

Depending on your situation, you can use either git rebase --onto (see this) or git cherry-pick (here). I'll follow up on this over slack.

You can create a branch called e.g. earth_occ_DC2 from the commit that has the Earth occultation code, but before you made these other changes, and maybe open a draft PR with it. I agree we should keep it around as an example and discuss it when we work out the actual code that works for the general, but no need to merge it now.

hiyoneda commented 1 month ago

Thanks a lot. I am closing this PR. I have made a draft PR by removing unrelated changes as #184.