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

Fix wcs.array_shape definition in WcsGeom.create #4677

Merged
merged 6 commits into from Oct 13, 2023

Conversation

QRemy
Copy link
Contributor

@QRemy QRemy commented Jul 21, 2023

Fix wcs.array_shape definition in WcsGeom.create to fix the bug described in :
#4652 (comment)

@QRemy QRemy added backport-v1.0.x on-merge: backport to v1.0.x backport-v1.1.x on-merge: backport to v1.1.x bug labels Jul 21, 2023
@QRemy QRemy added this to To do in gammapy.maps via automation Jul 21, 2023
@QRemy QRemy added this to the 1.0.2 milestone Jul 21, 2023
Copy link
Member

@Astro-Kirsty Astro-Kirsty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @QRemy.
Are the tests failing because of the known CI issue, or because this reverses the order where we have no implemented that before?

@registerrier
Copy link
Contributor

registerrier commented Jul 26, 2023

Thanks @QRemy . I am afraid the problems run deeper.

For instance, the get_shape function used in WcsGeom.create handles differently tuples and lists.

See:

>>> print(WcsGeom.create(binsz=(0.1,0.05)).wcs)
WCS Keywords

Number of WCS axes: 2
CTYPE : 'RA---CAR'  'DEC--CAR'  
CRVAL : 0.0  0.0  
CRPIX : 1800.5  1800.5  
PC1_1 PC1_2  : 1.0  0.0  
PC2_1 PC2_2  : 0.0  1.0  
CDELT : -0.1  0.05  
NAXIS : 3600  3600

but:

>>> print(WcsGeom.create(binsz=[0.1,0.05]).wcs)
WCS Keywords

Number of WCS axes: 2
CTYPE : 'RA---CAR'  'DEC--CAR'  
CRVAL : 0.0  0.0  
CRPIX : 1800.5  900.5  
PC1_1 PC1_2  : 1.0  0.0  
PC2_1 PC2_2  : 0.0  1.0  
CDELT : -0.1  0.1  
NAXIS : 1800  3600

EDIT: actually, this is probably the intended behaviour, because lists can define different bin size along non-spatial axes. I think this should be better protected since here there is no non-spatial axis.

@registerrier
Copy link
Contributor

According to https://astropy-astrofrog.readthedocs.io/en/latest/api/astropy.wcs.WCS.html#astropy.wcs.WCS.array_shape

the expected order is (rows, columns). It should then be reversed as you propose.

Signed-off-by:  <quentin.remy@live.fr>
Signed-off-by:  <quentin.remy@live.fr>
Signed-off-by:  <quentin.remy@live.fr>
Signed-off-by:  <quentin.remy@live.fr>
@QRemy
Copy link
Contributor Author

QRemy commented Aug 7, 2023

The previous tests failures were related to a mismatch between the width of the region geom and their wcs array_shape, I added a fix to have this consistent.

@QRemy QRemy requested a review from adonath August 7, 2023 16:33
Signed-off-by:  <quentin.remy@live.fr>
Signed-off-by:  <quentin.remy@live.fr>

adapt test

Signed-off-by:  <quentin.remy@live.fr>

adapt test

Signed-off-by:  <quentin.remy@live.fr>
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #4677 (78fa8d1) into main (60f03ad) will increase coverage by 0.00%.
The diff coverage is 86.66%.

@@           Coverage Diff           @@
##             main    #4677   +/-   ##
=======================================
  Coverage   75.95%   75.96%           
=======================================
  Files         223      223           
  Lines       31959    31969   +10     
=======================================
+ Hits        24275    24284    +9     
- Misses       7684     7685    +1     
Files Changed Coverage Δ
gammapy/modeling/models/tests/test_core.py 79.74% <0.00%> (ø)
gammapy/maps/wcs/geom.py 95.93% <83.33%> (-0.19%) ⬇️
gammapy/maps/region/geom.py 91.06% <100.00%> (+0.05%) ⬆️
gammapy/maps/wcs/tests/test_geom.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@registerrier registerrier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @QRemy. Looks good. See few inline comments.

# TODO : can we get the width before defining the wcs ?
wcs = WcsGeom.create(
binsz=binsz_wcs,
width=tuple(self.width),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed here? Why does it need to be re-created?

gammapy/modeling/models/tests/test_core.py Show resolved Hide resolved
gammapy/maps/wcs/geom.py Show resolved Hide resolved
Copy link
Contributor

@registerrier registerrier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @QRemy . Merging now.

@registerrier registerrier merged commit e23cc80 into gammapy:main Oct 13, 2023
15 checks passed
gammapy.maps automation moved this from To do to Done Oct 13, 2023
meeseeksmachine pushed a commit to meeseeksmachine/gammapy that referenced this pull request Oct 13, 2023
meeseeksmachine pushed a commit to meeseeksmachine/gammapy that referenced this pull request Oct 13, 2023
@registerrier registerrier modified the milestones: 1.0.2, 1.2 Oct 13, 2023
registerrier added a commit that referenced this pull request Oct 13, 2023
…7-on-v1.0.x

Backport PR #4677 on branch v1.0.x (Fix wcs.array_shape  definition in WcsGeom.create)
registerrier added a commit that referenced this pull request Oct 13, 2023
…7-on-v1.1.x

Backport PR #4677 on branch v1.1.x (Fix wcs.array_shape  definition in WcsGeom.create)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v1.0.x on-merge: backport to v1.0.x backport-v1.1.x on-merge: backport to v1.1.x bug
Projects
gammapy.maps
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants