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 IRFStacker class #762

Merged
merged 3 commits into from Nov 11, 2016
Merged

Add IRFStacker class #762

merged 3 commits into from Nov 11, 2016

Conversation

@JouvinLea
Copy link

@JouvinLea JouvinLea commented Nov 10, 2016

@joleroi
As we discussed I implemented the IRFstack class.
I already changed the method in the ObservationStacker class for the spectrum and add a test on the stacked arf and stacked rmf. I checked by hand that this is what you expected. I think this test in spectrum/test/test_observation is already a test for this class.
Cheers,
Lea

Copy link
Contributor

@joleroi joleroi left a comment

Thanks a lot for adding this. I left some minor comments mostly dealing with documentation. I think you accidentally added a copy of irf_stack.py in gammapy/irfs/test. This should not be there, right?

log = logging.getLogger(__name__)


class IRFstack(object):

This comment has been minimized.

@joleroi

joleroi Nov 10, 2016
Contributor

class names in gammapy are usually written in CamelCase. So in this case I would propose IRFStacker analogous to SpectrumObservationStacker.

Parameters
----------
list_arf: list
list of arf

This comment has been minimized.

@joleroi

joleroi Nov 10, 2016
Contributor

It's clearer what the user has to input if you write

list_arf : list
    list of `~gammapy.irf.EffectiveAreaTable`

the same hold for the other input parameters. In the documentation this will generate a link to the correct class. see for example: http://docs.gammapy.org/en/latest/api/gammapy.spectrum.SpectrumObservation.html#gammapy.spectrum.SpectrumObservation

list of arf
list_livetime: list
list of livetime
list_rmf: list

This comment has been minimized.

@joleroi

joleroi Nov 10, 2016
Contributor

To make it clear that this is an optional parameter, we usually write

list_rmf : list, optional
   etc.
"""
Compute the mean arf for several observations weighted by the livetime.
Create a new `~gammapy.irf.EffectiveAreaTable` with the mean arf

This comment has been minimized.

aeff_data = self.list_arf[i].evaluate(fill_nan=True)
aefft_current = aeff_data * self.list_livetime[i]
aefft += aefft_current
if (not self.list_low_threshold) & (not self.list_high_threshold):

This comment has been minimized.

@joleroi

joleroi Nov 10, 2016
Contributor

I don't think it's necessary to give such a detailed logging output. Also you should raise an error, because line 87 will not work anyways if no thresholds are present (and thus edisp_data is not defined).
IMO you don't need to check for self.list_low_threshold and self.list_high_threshold at all since this class will be mostly used by other classes or expert users, so people should now what they are doing (for example you also don't check in the __init__ method if all inputs are of the correct type, if we check for everything gammapy will be a huge checking machine).
However, if you want to add a statement you could maybe write

if not self.list_low_threshold or not self.list_high_threshold
    raise ValueError('Error message')
@joleroi
Copy link
Contributor

@joleroi joleroi commented Nov 10, 2016

I think this test in spectrum/test/test_observation is already a test for this class.

I agree

@cdeil cdeil added this to the 0.5 milestone Nov 10, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Nov 10, 2016

@joleroi - thanks for doing the review, I'm unsubscribing from this PR now.

FYI: I've added the "feature" label and removed the "infrastructure" label, and set the milestone to 0.5.
I use "infrastructure" for things like continuous integration or docs build issue.
And by default please always set the milestone to the next release.
If the pull request takes longer, we can always move it to the next release milestone.
I use milestones to fill the changelog before a release, so all issues or pull requests should get a milestone.

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Nov 10, 2016

@joleroi
I took into account you comments! For the docstring I don't know how to put the formula in the center

@joleroi
Copy link
Contributor

@joleroi joleroi commented Nov 10, 2016

Thanks, the remaining failure are due to sphinx issues. I can take care of that.

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Nov 10, 2016

great!
Le 10 nov. 2016 à 15:33, Johannes King notifications@github.com a écrit :

Thanks, the remaining failure are due to sphinx issues. I can take care of that.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@joleroi
Copy link
Contributor

@joleroi joleroi commented Nov 11, 2016

@JouvinLea I am merging this now since all checks pass. Note that I slightly changed the API of the IRFStacker class in my last PR. The methods are now called stack_edisp and stack_aeff to be consistent with the SpectrumObservationStacker.

@joleroi joleroi merged commit dd16ffb into gammapy:master Nov 11, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil cdeil changed the title add IRFstack Class Add IRFStacker class Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants