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

Move make_psf, make_mean_psf and make_mean_edisp #1862

Merged
merged 1 commit into from Oct 5, 2018

Conversation

@dcfidalgo
Copy link
Contributor

@dcfidalgo dcfidalgo commented Oct 5, 2018

As discussed today during the Coding Sprint, we want to move the IRF reduction away from the observation classes.
I moved them to a new file in irf/irf_reduce.py. For an easier review process, in this PR i just created the new file and copy/pasted the methods to it. As soon as this PR gets in, i will make a follow-up PR in which i change the methods to work as stand-alone functions.

@adonath
Copy link
Member

@adonath adonath commented Oct 5, 2018

Thanks, @dcfidalgo! As the change is rather straight forward, there is not much to review here. If you prefer, I can merge this PR and you continue in a separate one. But it's also fine by me, if you just continue your work in this PR.

Loading

@adonath adonath added the cleanup label Oct 5, 2018
@adonath adonath added this to To do in gammapy.irf via automation Oct 5, 2018
@adonath adonath added this to the 0.9 milestone Oct 5, 2018
@adonath adonath self-assigned this Oct 5, 2018
@dcfidalgo
Copy link
Contributor Author

@dcfidalgo dcfidalgo commented Oct 5, 2018

Could you maybe just merge this one? I would prefer to open up a new PR afterwards, then you get a nicer diff for the review :)

Loading

@adonath adonath self-requested a review Oct 5, 2018
adonath
adonath approved these changes Oct 5, 2018
Copy link
Member

@adonath adonath left a comment

I have no further comments.

Loading

@adonath adonath merged commit 45bc426 into gammapy:master Oct 5, 2018
3 of 4 checks passed
Loading
gammapy.irf automation moved this from To do to Done Oct 5, 2018
__all__ = ["make_psf", "make_mean_psf", "make_mean_edisp"]


def make_psf(self, position, energy=None, rad=None):
Copy link
Member

@cdeil cdeil Oct 5, 2018

Choose a reason for hiding this comment

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

Please rename self to obs here, and to add it to the Parameters list in the docstring.

self is just for methods on classes, shouldn't be used for standalone functions.

Same change for the functions below, just in the last one call it observations.
(well, that's my preference, that we go for observations everywhere, currently we have a mix of observations and obs_list in Gammapy).

Loading

Copy link
Member

@cdeil cdeil Oct 5, 2018

Choose a reason for hiding this comment

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

Ah, sorry, I missed your comment that you want to do that in a follow-up PR.
Yes, good, of course, remove the methods on the obs classes and use these ones.

Loading

@dcfidalgo dcfidalgo deleted the move_reduced_irfs branch Oct 8, 2018
@cdeil cdeil changed the title Move make_psf, make_mean_psf and make_mean_edisp to separate file Move make_psf, make_mean_psf and make_mean_edisp Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
gammapy.irf
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants