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

Adapt SkyModel to evaluate on TimeMapAxis #4828

Merged
merged 13 commits into from Dec 5, 2023
Merged

Conversation

AtreyeeS
Copy link
Member

@AtreyeeS AtreyeeS commented Oct 19, 2023

Addresses remaining part of #4480 - adapt SkyModel.evaluate_geom() and SkyModel.integrate_geom() to work with a TimeMapAxis

@AtreyeeS AtreyeeS marked this pull request as draft October 19, 2023 12:05
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2c58cf3) 75.80% compared to head (38e3281) 75.83%.
Report is 66 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4828      +/-   ##
==========================================
+ Coverage   75.80%   75.83%   +0.03%     
==========================================
  Files         227      227              
  Lines       33315    33401      +86     
==========================================
+ Hits        25255    25331      +76     
- Misses       8060     8070      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@registerrier registerrier added feature coding sprint To be tackled during coding sprint labels Oct 19, 2023
@registerrier registerrier added this to the 1.2 milestone Oct 19, 2023
@AtreyeeS
Copy link
Member Author

I have adapted SkyModel.evaluate_geom to support a TimeMapAxis.
Support for integrate_geom needs #4859 to be addressed

This has the loop over axis bins, which is not nice. Any better suggestion will be much appreciated.

The nice thing is this can allow FluxPointsDataset to work with n-dim geom. For eg, fitting light curves need not go through the splitting into multiple datasets for each time bin.

Maybe we can merge this now, and I can adapt the FluxPointsDataset after this?
The integrate_geom functionality is more necessary for MapEvaluator, which is not planned to be supported at the moment

@AtreyeeS AtreyeeS marked this pull request as ready for review November 3, 2023 07:23
@AtreyeeS AtreyeeS marked this pull request as draft November 3, 2023 08:57
@AtreyeeS AtreyeeS marked this pull request as ready for review November 3, 2023 10:02
@registerrier
Copy link
Contributor

Hi @AtreyeeS , #4859 has been merged. Could you rebase?

@AtreyeeS
Copy link
Member Author

After rebasing, I adapt SkyModel.integrate_geom() as well.
#4480 can be closed once this PR is merged.

AtreyeeS and others added 11 commits December 1, 2023 15:47
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Copy link
Contributor

@QRemy QRemy left a comment

Choose a reason for hiding this comment

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

Thanks @AtreyeeS, looks good, I just pushed a commit to reduce code duplication and left one question :

gammapy/modeling/models/cube.py Outdated Show resolved Hide resolved
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
@QRemy QRemy self-assigned this Dec 1, 2023
Astro-Kirsty
Astro-Kirsty previously approved these changes Dec 5, 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 @AtreyeeS

gammapy/modeling/models/cube.py Outdated Show resolved Hide resolved
@QRemy QRemy merged commit 3e48292 into gammapy:main Dec 5, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coding sprint To be tackled during coding sprint feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants