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

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

Closed
wants to merge 10 commits into from

Conversation

hiyoneda
Copy link
Contributor

@hiyoneda hiyoneda commented May 31, 2024

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.

  • mask a part of the sky if such a region has zero exposure time.
  • add a parameter mininum_flux in deconvolution_algorithm_base in order not to make pixels with minus flux. The default value is 0.
  • modified the implementation of the function that calculates the acceleration parameter in the RL algorithm.
  • use hp.smoothing for the Gaussian image smoothing instead of preparing a large matrix.

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

@hiyoneda hiyoneda added enhancement New feature or request imaging labels May 31, 2024
@israelmcmc
Copy link
Collaborator

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

hiyoneda commented Jun 3, 2024

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

  • The change in the coordinate conversion matrix does not harm the DC2. While I added a new parameter, earth_horizon_angle, the old notebooks published for DC2 should work since earth_horizon_angle is not a mandatory parameter.
  • To consider the earth occultation effect in a general way, I understood that we need information about the satellite altitude and position in the orientation file. So, I think that we also need to modify the orientation. I think that this code can be a good example to understand and discuss it.

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

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 hiyoneda marked this pull request as draft June 3, 2024 15:22
@hiyoneda
Copy link
Contributor Author

hiyoneda commented Jun 3, 2024

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

@hiyoneda hiyoneda closed this Jun 3, 2024
@hiyoneda hiyoneda deleted the earth_occultation branch June 3, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request imaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants