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

Tweak in MapEvaluator.need_update for performance #3353

Merged
merged 10 commits into from
May 17, 2021

Conversation

QRemy
Copy link
Contributor

@QRemy QRemy commented May 7, 2021

This PR introduce a performance tweak in MapEvaluator.need_update that replaces skycoord.separation by angular_separation in order to avoid unnecessary frame and unit equivalence comparison.

@QRemy QRemy requested a review from adonath May 7, 2021 12:03
@QRemy QRemy added the cleanup label May 7, 2021
@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #3353 (af78a32) into master (4e1c72b) will decrease coverage by 0.02%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3353      +/-   ##
==========================================
- Coverage   93.72%   93.70%   -0.03%     
==========================================
  Files         144      144              
  Lines       18270    18282      +12     
==========================================
+ Hits        17124    17131       +7     
- Misses       1146     1151       +5     
Impacted Files Coverage Δ
gammapy/datasets/map.py 94.34% <88.23%> (-0.13%) ⬇️
gammapy/modeling/iminuit.py 96.73% <0.00%> (-3.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e1c72b...af78a32. Read the comment docs.

@adonath adonath added this to the v0.19 milestone May 7, 2021
@QRemy
Copy link
Contributor Author

QRemy commented May 7, 2021

Added a test to avoid update if there is no free spatial parameters (solve fail related to TemplateSpatialModel)

@adonath
Copy link
Member

adonath commented May 10, 2021

Thanks a lot @QRemy! One last suggestion to try: maybe it is an option to also check for if parameters_spatial_changed in .need_update and only check for the separation if this is the case?

@adonath
Copy link
Member

adonath commented May 12, 2021

Thanks @QRemy, it seems there are now test fails https://github.com/gammapy/gammapy/pull/3353/checks?check_run_id=2557044541#step:9:1865, I can also reproduce them locally....not sure what is happening. Maybe see if they can be fixed with limited effort...

Copy link
Member

@adonath adonath 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, the remaining test fails seems unrelated. I think adding the lon_0 and lat_0 parameters to the TemplateSpatialModel is still a good idea, however this can be done in a follow up PR.

@adonath adonath added performance Performance improvement and removed cleanup labels May 17, 2021
@adonath adonath merged commit 295cbb3 into gammapy:master May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants