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

Add 1LHAASO to gammapy.catalog #4595

Merged
merged 13 commits into from Jul 17, 2023
Merged

Add 1LHAASO to gammapy.catalog #4595

merged 13 commits into from Jul 17, 2023

Conversation

QRemy
Copy link
Contributor

@QRemy QRemy commented Jun 19, 2023

Add 1LHAASO catalog class.
This requires to add the catalog FITS file to gammapy-data.

@QRemy QRemy added the feature label Jun 19, 2023
@QRemy QRemy changed the title Add 1LHASSO to gammapy.catalog Add 1LHAASO to gammapy.catalog Jun 20, 2023
@QRemy QRemy added this to the 1.2 milestone Jun 21, 2023
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
@QRemy QRemy marked this pull request as ready for review June 26, 2023 16:45
Copy link
Member

@AtreyeeS AtreyeeS 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 ! I propose we merge this now, and add more support by the next release

gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
QRemy and others added 11 commits July 12, 2023 13:52
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>
Co-authored-by: Bruno Khélifi <khelifi@in2p3.fr>
Co-authored-by: Bruno Khélifi <khelifi@in2p3.fr>
Signed-off-by:  <quentin.remy@live.fr>
Signed-off-by:  <quentin.remy@live.fr>
Signed-off-by:  <quentin.remy@live.fr>
Co-authored-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by:  <quentin.remy@live.fr>
bkhelifi
bkhelifi previously approved these changes Jul 13, 2023
Copy link
Member

@bkhelifi bkhelifi left a comment

Choose a reason for hiding this comment

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

Thanks again, @QRemy

registerrier
registerrier previously approved these changes Jul 13, 2023
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 . This looks good. I have left a few minor inline comments.questions.

gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved

errs = {
"lat_0": pos_err / scale_r95,
"lon_0": pos_err / scale_r95 / np.cos(lat_0),
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the division by cos(lat) necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure it is the same for 4FGL and 3HWC catalogue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this was introduced after issue #2460 . I suppose this implies that the error in the table is given as a proper angular separation and one wants to correct for that. Does this apply all the time?

gammapy/catalog/lhaaso.py Show resolved Hide resolved
Co-authored-by: Régis Terrier <regis.terrier@m4x.org>
@QRemy QRemy dismissed stale reviews from registerrier and bkhelifi via f3a2719 July 14, 2023 08:15
@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #4595 (f3a2719) into main (fc886b9) will increase coverage by 0.02%.
The diff coverage is 98.91%.

❗ Current head f3a2719 differs from pull request most recent head aa9b59d. Consider uploading reports for the commit aa9b59d to get more accurate results

@@            Coverage Diff             @@
##             main    #4595      +/-   ##
==========================================
+ Coverage   95.05%   95.07%   +0.02%     
==========================================
  Files         221      222       +1     
  Lines       31573    31723     +150     
==========================================
+ Hits        30013    30162     +149     
- Misses       1560     1561       +1     
Impacted Files Coverage Δ
gammapy/catalog/lhaaso.py 98.87% <98.87%> (ø)
gammapy/catalog/__init__.py 100.00% <100.00%> (ø)
gammapy/catalog/core.py 95.23% <100.00%> (+0.06%) ⬆️

... and 7 files with indirect coverage changes

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

AtreyeeS
AtreyeeS previously approved these changes Jul 14, 2023
Copy link
Member

@AtreyeeS AtreyeeS left a comment

Choose a reason for hiding this comment

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

Thanks!

gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
bkhelifi
bkhelifi previously approved these changes Jul 17, 2023
Copy link
Member

@bkhelifi bkhelifi 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

gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
gammapy/catalog/lhaaso.py Outdated Show resolved Hide resolved
@QRemy QRemy dismissed stale reviews from bkhelifi and AtreyeeS via aa9b59d July 17, 2023 09:16
@QRemy QRemy merged commit 0a9323a into gammapy:main Jul 17, 2023
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
gammapy.catalog
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants