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

Activate PSFMap for fitting #2627

Merged
merged 5 commits into from Nov 29, 2019
Merged

Activate PSFMap for fitting #2627

merged 5 commits into from Nov 29, 2019

Conversation

@AtreyeeS
Copy link
Member

AtreyeeS commented Nov 28, 2019

Description

This pull request activates PSFMap on the MapEvaluator. Similar to what is done in #2587 , the option to keep PSFKernel is still on. Once this PR is merged, I will deactivate the kernels and adapt the notebooks.

Dear reviewer

One specific doubt I have is what should be the max_radius in
self.psf = psf.get_psf_kernel(self.model.position, geom=exposure.geom, max_radius=0.8 * u.deg)
Having it as a parameter will be extremely confusing for the users. Currently I am passing a large enough value which should hopefully suffice for most cases.

I did not adapt the tests in analysis to avoid conflicts with the changes @Bultako might be making.

@AtreyeeS AtreyeeS requested a review from adonath Nov 28, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 28, 2019

Codecov Report

Merging #2627 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2627      +/-   ##
=========================================
+ Coverage    91.5%   91.5%   +<.01%     
=========================================
  Files         141     141              
  Lines       15942   15944       +2     
=========================================
+ Hits        14588   14590       +2     
  Misses       1354    1354
Impacted Files Coverage Δ
gammapy/cube/fit.py 89.93% <100%> (+0.03%) ⬆️
gammapy/cube/simulate.py 97.72% <100%> (ø) ⬆️

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 e12d3eb...e0985d7. Read the comment docs.

@adonath adonath self-assigned this Nov 28, 2019
@adonath adonath added this to the 0.15 milestone Nov 28, 2019
@adonath adonath added the feature label Nov 28, 2019
@adonath adonath changed the title activate psf_map Activate PSFMap for fitting Nov 28, 2019
Copy link
Member

adonath left a comment

Thanks @AtreyeeS. This look good to me. I've left one comment concerning the max_radius parameter. Once this is addressed we can merge...

self.psf = psf # .get_psf_kernel(self.model.position, geom=exposure.geom)
if isinstance(psf, PSFMap):
self.psf = psf.get_psf_kernel(
self.model.position, geom=exposure.geom, max_radius=0.8 * u.deg

This comment has been minimized.

Copy link
@adonath

adonath Nov 28, 2019

Member

I would propose to modify .get_psf_kernel() and just take the maximum of table_psf.rad as a default maximum radius (if max_radius is None). This is more consistent then having a separate hard-coded value here...

@AtreyeeS

This comment has been minimized.

Copy link
Member Author

AtreyeeS commented Nov 29, 2019

Thanks @adonath ! I have made the change

@adonath

This comment has been minimized.

Copy link
Member

adonath commented Nov 29, 2019

Thanks @AtreyeeS! I've just restarted the remaining failing Travis builds, once those are green Ill merge...

@adonath

This comment has been minimized.

Copy link
Member

adonath commented Nov 29, 2019

Ok, it seems gammapy download is broken...the Travis-CI fails are unrelated. The tests pass locally for me. I'll go ahead and merge now...

@adonath adonath merged commit ef88663 into gammapy:master Nov 29, 2019
9 of 10 checks passed
9 of 10 checks passed
greeting
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
gammapy.gammapy #20191129.6 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.