Open gabemery opened 1 month ago
Thank you for the comments. I will implement them. @registerrier The default value was not motivated. As you said it should depend on the FoV radius and bin size (reducing to the number of pixels).
A quick estimate is :
Deviation at R for rotation of d_theta: 2 pi R * d_theta / 360 deg size of pixel for a FOV of radius R and n_pix pixels per dimension: 2R/n_pix
If we want the deviation to be less than the size of a pixel: 2 pi R * d_theta / 360 deg < 2R/n_pix d_theta < 720 deg / (2 pi npix) taking n_pix ~ 100 and using 2 pi ~ 7.2 we have d_theta <~ 1 deg
So using this logic, 0.5 deg is not bad. But I would be OK with changing this value.
@gabemery Is this new feature urgent or can it go into v2.0?
Regarding the default value I hame made a few tests using your calculation of the minimum angle for 2, 1 and 0.5 degree steps with a bar like bkg IRF (0.02 deg image binning):
It seems to me that 1 deg still give relatively good results up to 10 deg offset.
@registerrier I don't think it is urgent. We need it in my team but we can work without a numbered release, at least for the near future.
I can change the default to 1 deg.
Attention: Patch coverage is 85.18519%
with 4 lines
in your changes missing coverage. Please review.
Project coverage is 94.60%. Comparing base (
1545062
) to head (27c659f
). Report is 1 commits behind head on main.
Files with missing lines | Patch % | Lines |
---|---|---|
gammapy/makers/utils.py | 84.61% | 4 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hello @gabemery, can you update your branch and fix the tests?
@bkhelifi It is done.
One of the tests uses a deprecated feature of not passing obstime
which ends up as None. I added a workaround and comments with TODO to identify it.
I am unsure why "CI / ubuntu-latest, devdeps (pull_request)" fails and how to solve it. Local tests went further but still failed at scipy :
Collecting scipy>=0.0.dev0
Running command git clone --filter=blob:none --quiet https://github.com/scikit-hep/iminuit.git /tmp/pip-req-build-uejt8qd2
Running command git submodule update --init --recursive -q
ERROR: HTTP error 404 while getting https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/1.15.0.dev0/scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (from https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/)
ERROR: Could not install requirement scipy>=0.0.dev0 from https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/1.15.0.dev0/scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl because of HTTP error 404 Client Error: Not Found for url: https://binstar-cio-packages-prod.s3.amazonaws.com/64774d9554a6f0c49626f52d/670259440893138e9b5b9aaa?response-content-disposition=attachment%3B%20filename%3D%22scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl%22%3B%20filename%2A%3DUTF-8%27%27scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl&response-content-type=application%2Foctet-stream&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIAWUI46DZFGU7XD2YL%2F20241007%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20241007T103727Z&X-Amz-Expires=600&X-Amz-SignedHeaders=host&X-Amz-Security-Token=IQoJb3JpZ2luX2VjENP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLWVhc3QtMSJHMEUCIQCtOqhQgfRhUUvYJ%2BKRFUynSogLbvzUebyTCL7A6AmATAIgVzwagrszpPaCPJmoYeJUxhndrCFSwdxkVSYoEa9e4XYq%2FgQIKxAAGgw0NTU4NjQwOTgzNzgiDD6GGq72VRG9MzMQfirbBDhA3H00YC%2BVUILumVFYrKlNepONx9TVnpLhPmkiGn%2FV94ksvMusCoZ7cUKEPAW6yHGD6GDI4%2FutoHkh8CTrc%2FNXiYMcdPPWiQ%2FwQM67xXub4C%2BVMT8L3rpuWCNohmJPWkgO70csiMo0bRIbKnw4OTdLuqnQbtGageOheh%2FIowibhTnwM3HwxxqRLlMtbE%2Fq8quawB3vpmVqiLxEi9xgswePgNjSVyUL6uGg4WAKMnTK7wyPePQ3jhgRlgA5mutCNk6gFb7Ts85iZxpfPe5vdrQ5zdZSzVGF%2FS1WAqzmeJ1zPF0hxWzKW4djurFgw5iM3xACzc7AjswvUeD53DIVHCuttcZ2s17rItEMr8nwtEyL68SQcpsclrryet2ca0hPbsFAFBem5tIRojHxYN%2FPUP5XJnqVCoKVT8%2Fg6BUEWbU3HMcFRg8jkye18NXvKF%2FGHeMD6l1iuFIXEL6WQoY4yjc344HGHwVnrOhuMNw8tH%2F3nZw6ybWOrpIVegE9XWqDAczvXb5IHT7yl%2FgJZCi685Pggpg9ta7q%2FlhYEYEZEBIIozRPfD0kZxrdlDXDur43ROJAwJIZXLuK0Ty3g5BFf98yv3kM48y%2F0WdpKbRrBY9GY7coXxxMvYBhd5Y4BXxORPfmONN3zl1SyMcPjHKDkvhmoNnGayXGt%2BXh%2BDEuIHQtPiuFqC7ksaQbftKAoZpuiDR74d1dQ5oBoq7gBr0VV%2FnwjwiwCmUIAjqQV2%2ByQzBAD55oyel%2BQxnArYwp7fZsvjK7VXZbjgc9z5Lqq3O5eJF%2FSOHFA1PXNvuvdTC46Y64BjqaAWaxtu7SulQyCabeE0PKVPejTklPivU%2BMs8xZ3qlyYh1%2FZ82PU8SOaxb%2BL%2BL5IPcgYcG6WzjLEHnMdTS6onhL6PYYChFs0IwcDy%2FQbBep6Bxwvhs6Q1VGbBxdUu%2FdEXR4Uft9O3arpewD9s4F8KFKbQd0MqRQqAG%2BLlXgt9AatM6DwMyIVcz0IGL%2BhMcQR%2BmuMOHmj6xa6dzSDc%3D&X-Amz-Signature=f0cd6c5cc3dd2d6669d6b26d8cfeb4309066616a190d9694d1bb0cd63dcd6afe for URL https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/1.15.0.dev0/scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (from https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/)
devdeps: exit 1 (171.82 seconds) /home/emery/Documents/dev_gammapy> python -I -m pip install 'astropy>=0.0.dev0' 'matplotlib>=0.0.dev0' 'numpy>=0.0.dev0' 'pyerfa>=0.0.dev0' 'scipy>=0.0.dev0' git+https://github.com/scikit-hep/iminuit.git pid=209423
devdeps: FAIL code 1 (172.00 seconds)
evaluation failed :( (172.09 seconds)
@bkhelifi It is done. One of the tests uses a deprecated feature of not passing
obstime
which ends up as None. I added a workaround and comments with TODO to identify it.I am unsure why "CI / ubuntu-latest, devdeps (pull_request)" fails and how to solve it. Local tests went further but still failed at scipy :
Collecting scipy>=0.0.dev0 Running command git clone --filter=blob:none --quiet https://github.com/scikit-hep/iminuit.git /tmp/pip-req-build-uejt8qd2 Running command git submodule update --init --recursive -q ERROR: HTTP error 404 while getting https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/1.15.0.dev0/scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (from https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/) ERROR: Could not install requirement scipy>=0.0.dev0 from https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/1.15.0.dev0/scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl because of HTTP error 404 Client Error: Not Found for url: https://binstar-cio-packages-prod.s3.amazonaws.com/64774d9554a6f0c49626f52d/670259440893138e9b5b9aaa?response-content-disposition=attachment%3B%20filename%3D%22scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl%22%3B%20filename%2A%3DUTF-8%27%27scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl&response-content-type=application%2Foctet-stream&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIAWUI46DZFGU7XD2YL%2F20241007%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20241007T103727Z&X-Amz-Expires=600&X-Amz-SignedHeaders=host&X-Amz-Security-Token=IQoJb3JpZ2luX2VjENP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLWVhc3QtMSJHMEUCIQCtOqhQgfRhUUvYJ%2BKRFUynSogLbvzUebyTCL7A6AmATAIgVzwagrszpPaCPJmoYeJUxhndrCFSwdxkVSYoEa9e4XYq%2FgQIKxAAGgw0NTU4NjQwOTgzNzgiDD6GGq72VRG9MzMQfirbBDhA3H00YC%2BVUILumVFYrKlNepONx9TVnpLhPmkiGn%2FV94ksvMusCoZ7cUKEPAW6yHGD6GDI4%2FutoHkh8CTrc%2FNXiYMcdPPWiQ%2FwQM67xXub4C%2BVMT8L3rpuWCNohmJPWkgO70csiMo0bRIbKnw4OTdLuqnQbtGageOheh%2FIowibhTnwM3HwxxqRLlMtbE%2Fq8quawB3vpmVqiLxEi9xgswePgNjSVyUL6uGg4WAKMnTK7wyPePQ3jhgRlgA5mutCNk6gFb7Ts85iZxpfPe5vdrQ5zdZSzVGF%2FS1WAqzmeJ1zPF0hxWzKW4djurFgw5iM3xACzc7AjswvUeD53DIVHCuttcZ2s17rItEMr8nwtEyL68SQcpsclrryet2ca0hPbsFAFBem5tIRojHxYN%2FPUP5XJnqVCoKVT8%2Fg6BUEWbU3HMcFRg8jkye18NXvKF%2FGHeMD6l1iuFIXEL6WQoY4yjc344HGHwVnrOhuMNw8tH%2F3nZw6ybWOrpIVegE9XWqDAczvXb5IHT7yl%2FgJZCi685Pggpg9ta7q%2FlhYEYEZEBIIozRPfD0kZxrdlDXDur43ROJAwJIZXLuK0Ty3g5BFf98yv3kM48y%2F0WdpKbRrBY9GY7coXxxMvYBhd5Y4BXxORPfmONN3zl1SyMcPjHKDkvhmoNnGayXGt%2BXh%2BDEuIHQtPiuFqC7ksaQbftKAoZpuiDR74d1dQ5oBoq7gBr0VV%2FnwjwiwCmUIAjqQV2%2ByQzBAD55oyel%2BQxnArYwp7fZsvjK7VXZbjgc9z5Lqq3O5eJF%2FSOHFA1PXNvuvdTC46Y64BjqaAWaxtu7SulQyCabeE0PKVPejTklPivU%2BMs8xZ3qlyYh1%2FZ82PU8SOaxb%2BL%2BL5IPcgYcG6WzjLEHnMdTS6onhL6PYYChFs0IwcDy%2FQbBep6Bxwvhs6Q1VGbBxdUu%2FdEXR4Uft9O3arpewD9s4F8KFKbQd0MqRQqAG%2BLlXgt9AatM6DwMyIVcz0IGL%2BhMcQR%2BmuMOHmj6xa6dzSDc%3D&X-Amz-Signature=f0cd6c5cc3dd2d6669d6b26d8cfeb4309066616a190d9694d1bb0cd63dcd6afe for URL https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/1.15.0.dev0/scipy-1.15.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (from https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy/) devdeps: exit 1 (171.82 seconds) /home/emery/Documents/dev_gammapy> python -I -m pip install 'astropy>=0.0.dev0' 'matplotlib>=0.0.dev0' 'numpy>=0.0.dev0' 'pyerfa>=0.0.dev0' 'scipy>=0.0.dev0' git+https://github.com/scikit-hep/iminuit.git pid=209423 devdeps: FAIL code 1 (172.00 seconds) evaluation failed :( (172.09 seconds)
An issue with the latest version of scipy. Let's wait few days.... The other tests work, so one can work on the review already;)
Description
This aims at improving the
make_map_background_irf
method when the background IRF is 3d with an AltAz alignment. This covers the first point of issue #4860 (and the associated TODO in the code) but with a different logic than the one mentioned there.Current limitation : the
make_map_background_irf
used to evaluate the background counts in RaDec applies a single frame transformation when using a background IRF defined in AltAz alignment. This means that if the field of view rotates during the observation, the background will be miss estimated if it is not rotation symmetric.Proposed solution : Re-evaluate the background IRF on smaller time intervals. The original proposal was to use fixed time intervals. I have instead implemented a solution where the time intervals are created based on a maximum error on the FOV rotation.
Logic of the criteria : The rotation rate of the Radec frame in AltAz is a function of the observatory latitude, and pointing in altaz. Using this first order derivative, a given limit on rotation can be converted in a maximum observation time. It should be valid as long as the rotation rate doesn't change too fast during the output time window.
Dear reviewer This pull request is ready for review.
Should I add some documentation?