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

Pacman #461

Merged
merged 24 commits into from Feb 24, 2016

Conversation

Projects
None yet
2 participants
@JouvinLea
Contributor

JouvinLea commented Feb 18, 2016

This PR implements livetime and counts correction for Background Model production to take into account the presence of agn with the Pacman method.

@cdeil cdeil added the feature label Feb 18, 2016

@cdeil cdeil added this to the 0.4 milestone Feb 18, 2016

@cdeil cdeil self-assigned this Feb 18, 2016

@JouvinLea JouvinLea force-pushed the JouvinLea:Packman branch from 261c99d to 34d8f8c Feb 18, 2016

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 19, 2016

@JouvinLea - I'll make the Gammapy 0.4 release on Sunday.
Having this included isn't critical, just FYI in case you do want to get this in.

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 20, 2016

I'm moving this PR to the 0.5 milestone, there's no hurry on this.

@cdeil cdeil modified the milestones: 0.5, 0.4 Feb 20, 2016

assert_allclose(pie_fraction, pie_fraction_expected)
def test_select_events_outside_pie():

This comment has been minimized.

@JouvinLea

JouvinLea Feb 21, 2016

Contributor

For this test, I joined the image with source to exclude in the last commit

This comment has been minimized.

@cdeil

cdeil Feb 21, 2016

Member

@JouvinLea - Please remove the gammapy/background/tests/pie.tiff file.

We decided to remove all test data files from the code repo, and to put them in the gammapy-extra repo instead:
https://github.com/gammapy/gammapy-extra/tree/master/test_datasets

Also -- please use FITS files for images, not TIFF.

And is it really useful to store the file in the repo? If it's quick and simple to generate, you can use a pytest.fixture to do this on the fly, when the tests are running.

This comment has been minimized.

@JouvinLea

JouvinLea Feb 21, 2016

Contributor

O, I removed the .tiff. If I save in fits format we don't see the region this is why I used fits. But you can see it if you open the image with the source.reg file!
Should I move the fits image and the source.reg file in gammapy-extra?

This comment has been minimized.

@cdeil

cdeil Feb 21, 2016

Member

Should I move the fits image and the source.reg file in gammapy-extra?

I don't understand yet ... you're not accessing the image and region file from the test, right?
So is this image just a check you did on the side, not part of the test committed here?

If so, how about putting the image, region file and script you ran in a gammapy-extra/checks/pacman/ folder?

This comment has been minimized.

@JouvinLea

JouvinLea Feb 21, 2016

Contributor

yeah I will do that

This comment has been minimized.

@cdeil

cdeil Feb 21, 2016

Member

And if you're not reading the image from the Gammapy tests, I also don't care much about the image format.
TIFF, PNG, ... is fine.

I just don't want this as part of the official tests, because a TIFF image reader isn't available for every user that wants to run the tests.

This comment has been minimized.

@JouvinLea

JouvinLea Feb 23, 2016

Contributor

Create a new test with some events I know are outside or inside the pie ok?

This comment has been minimized.

@cdeil

cdeil Feb 23, 2016

Member

Yes.

multi_array.compute_rate()
assert_equal(multi_array.counts.data.value.sum(), 5403)
pix = 23, 1
assert_quantity_allclose(multi_array.livetime.data[pix], 6313.8117676 * u.s)
rate = Quantity(0.0024697306536062276, "MeV-1 s-1 sr-1")
assert_quantity_allclose(multi_array.bg_rate.data[pix], rate)
def test_fillobs_pie(self):

This comment has been minimized.

@JouvinLea

JouvinLea Feb 21, 2016

Contributor

Tell me what you think about these tests!!

This comment has been minimized.

@cdeil

cdeil Feb 21, 2016

Member

@JouvinLea - I still have to prepare a presentation for tomorrow.

From my side, the most important points are:

  • the high-level example that builds the background model should "work", i.e. the peaks at 0.5 and 1.5 deg in the acceptance curve should be gone
  • there should be a test that covers the new functionality that passes on travis-ci.

As long as that's the case, I'm happy to merge if you want.
Of course, if you want a thorough code review and feedback, I can do that, but only tomorrow afternoon.

(I'll probably have to delay the Gammapy 0.4 release for a day or two because there's too much cleanup / outstanding little issues that are work in progress.)

This comment has been minimized.

@JouvinLea

JouvinLea Feb 21, 2016

Contributor

Yeah no problem there is no hurry for this. It's just I pushed what I did!
Could be great just to have a feedback about the quality of the tests but again there is no hurry. We will see if it passes on travis.

Lea Jouvin added some commits Feb 21, 2016

Lea Jouvin
deleted image
 Please enter the commit message for your changes. Lines starting
Lea Jouvin
pass
def add_column_and_sort_table(sources, pointing_position):

This comment has been minimized.

@cdeil

cdeil Feb 22, 2016

Member

The sources.sort call means that the table a user puts in will be modified.

It's usually better to not modify inputs, but only to compute something and return a new object.
If functions modify inputs, that's often surprising and hard to debug for users / callers.

So in this case, you should add a line that makes a copy of the input table at the top:

sources = sources.copy()
@@ -24,6 +24,91 @@
DEFAULT_SPLINE_KWARGS = dict(k=1, s=0)
def _select_closest():

This comment has been minimized.

@cdeil

cdeil Feb 22, 2016

Member

Did you want to implement _select_closest or should this be removed?

Returns
-------
pie fraction : float

This comment has been minimized.

@cdeil

cdeil Feb 22, 2016

Member

Add a line to explain what it is (0 = nothing excluded, right?)

Field of view radius
Returns
-------

This comment has been minimized.

@cdeil

cdeil Feb 22, 2016

Member

Document what this returns.

phi_max = phi_source + np.arctan(radius / separation)
skycoord_pix = SkyCoord(ra, dec, unit='deg')
phi = pointing_position.position_angle(skycoord_pix)
idx_image_out_pie = np.where((phi > phi_max) | (phi < phi_min))

This comment has been minimized.

@cdeil

cdeil Feb 22, 2016

Member

You're re-implementing the algorithm in the test.
That's problematic ... it's a lot of complex extra code to read / maintain.
And if there's a bug in the actual function, there's a good chance you'd make the same error when writing the test.

I do think it's good that you've done this check and manually verified that you get a pie image with this function!

But for the test here, maybe just put a few positions (a handful or less) where you know they are inside or outside the pie and assert that you get the right answer?

I know we talked about the image as a test before.
But do you agree that re-implementing the algorithm in the test is bad?

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 22, 2016

Does the test_curve.py example work?
Can you paste the acceptance curve that is output here?
Is it as expected, i.e. the peaks at 0.5 and 1.5 deg are gone?

@cdeil cdeil modified the milestones: 0.4, 0.5 Feb 22, 2016

Lea Jouvin added some commits Feb 23, 2016

Lea Jouvin
Lea Jouvin
@JouvinLea

This comment has been minimized.

Contributor

JouvinLea commented Feb 23, 2016

I added the crab as a source in the test_curve and I put the acceptance curve in gammapy-extra/check/pacman in the new PR.
I think yes the peaks disappeared

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 23, 2016

OK, let me know if you have any questions, or when this is ready to be merged.

Lea Jouvin added some commits Feb 23, 2016

Lea Jouvin
Lea Jouvin
@@ -24,6 +24,92 @@
DEFAULT_SPLINE_KWARGS = dict(k=1, s=0)
def add_column_and_sort_table(sources, pointing_position):

This comment has been minimized.

@JouvinLea

JouvinLea Feb 23, 2016

Contributor

Just I don't know how to make these methods appear in the docstring?

This comment has been minimized.

@cdeil

cdeil Feb 23, 2016

Member

To make a function or class appear in the API docs, you have to add it to the __all__ list at the top of the file.

In this case though I think it's more an internal helper function that doesn't need to be advertised to end users as part of the public Gammapy API, i.e. my suggestion would be to not add it to __all__ and maybe even to add an underscore to the function name to make it clear it's a private helper function: _add_column_and_sort_table (the underscore is the common patter to mark things private in Python.

@JouvinLea - What do you think? OK to keep this private?

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 24, 2016

@leajouvin - OK if I merge this?

(I want to re-use the EnergyOffsetArray class for the PSF and make some small changes: #447)

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 24, 2016

@leajouvin - FYI - I think we should change the axis order for EnergyOffsetArray.
At the moment we have ENERGY, THETA:

>>> t = Table.read('/Users/deil/temp/test-bg/out/background_2D_group_000_table.fits.gz')
>>> t.info()
<Table length=1>
  name    dtype    shape        unit     
-------- ------- --------- --------------
THETA_LO float64     (99,)            deg
THETA_HI float64     (99,)            deg
ENERG_LO float64    (100,)            TeV
ENERG_HI float64    (100,)            TeV
  counts float64 (100, 99)               
livetime float64 (100, 99)              s
 bg_rate float64 (100, 99) 1 / (MeV s sr)

But effective area and PSF are stored with opposite order THETA, ENERGY:

>>> t = Table.read('pa-example-files//run23400-23599/run23523/aeff_2d_23523.fits.gz')
>>> t.info()
<Table length=1>
    name      dtype   shape  unit
------------ ------- ------- ----
    ENERG_LO float32   (15,)  TeV
    ENERG_HI float32   (15,)  TeV
    THETA_LO float32    (6,)  deg
    THETA_HI float32    (6,)  deg
     EFFAREA float32 (6, 15)   m2
EFFAREA_RECO float32 (6, 15)   m2

Given that PSF and AEFF are already in use with order ENERGY, THETA I think we should just adapt BKG_2D.

I'd like to do this change myself in #447.
Just let me know if you have any further commits you want to push here, or if I can merge now.

@cdeil cdeil referenced this pull request Feb 24, 2016

Merged

Specify IRF axis orders #28

Lea Jouvin
@JouvinLea

This comment has been minimized.

Contributor

JouvinLea commented Feb 24, 2016

Hi,
Yes I agree! I added the _ so if it passes the test on github it's ready to merge!

@JouvinLea

This comment has been minimized.

Contributor

JouvinLea commented Feb 24, 2016

I'm ok to change the axis order! Tell me when it will be done I will have to change my script!

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 24, 2016

I'm merging this now.
If there's any issues I'll fix in the follow-up PR where I also change the axis order in the FITS file.

@JouvinLea

This comment has been minimized.

Contributor

JouvinLea commented Feb 24, 2016

ok!!

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 24, 2016

Thanks!

image

cdeil added a commit that referenced this pull request Feb 24, 2016

@cdeil cdeil merged commit 6b4276c into gammapy:master Feb 24, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment