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

Fix image pipe class #521

Merged
merged 16 commits into from Apr 26, 2016
Merged

Fix image pipe class #521

merged 16 commits into from Apr 26, 2016

Conversation

@JouvinLea
Copy link

@JouvinLea JouvinLea commented Apr 22, 2016

Try to make the class works again...
For the moment I am using the script test_curve.py in gammapy/example to test. I will do a test when I would have fix the bug

@cdeil cdeil added this to the 0.5 milestone Apr 22, 2016
@cdeil cdeil self-assigned this Apr 22, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Apr 22, 2016

Thanks! Let me know if you have any questions or want me to look at something.

Loading

@@ -260,8 +260,8 @@ def solid_angle(self):
"""
Solid angle image
"""
xsky, ysky = self.coordinates(mode='edges')
Copy link
Author

@JouvinLea JouvinLea Apr 22, 2016

Choose a reason for hiding this comment

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

@cdeil
Get an error:

ValueError: too many values to unpack

with the old version. You changed the coordinates method of the skymap that return a tuple if coord_type=galactic but an array if coord_type=skycoord.
Is it ok what I did?

Loading

Copy link
Member

@cdeil cdeil Apr 22, 2016

Choose a reason for hiding this comment

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

@adonath - Can you have a look and answer this question?

Loading

@@ -45,6 +45,10 @@ def test_coordinates(self):
assert_allclose(coordinates[0][100, 100].degree, 0.01)
assert_allclose(coordinates[1][100, 100].degree, 0.01)

def test_solid_angle(self):
Copy link
Author

@JouvinLea JouvinLea Apr 23, 2016

Choose a reason for hiding this comment

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

@adonath
add this test for the solid angle, why is it negative?

Loading

Copy link
Member

@adonath adonath Apr 25, 2016

Choose a reason for hiding this comment

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

That's strange...it shouldn't be negative and it isn't the case for the test files I used. But let's change it to make it return safe results...

Loading

@cdeil cdeil assigned adonath and unassigned cdeil Apr 25, 2016
@@ -260,7 +260,7 @@ def solid_angle(self):
"""
Solid angle image
"""
xsky, ysky = self.coordinates(mode='edges')
xsky, ysky = self.coordinates(coord_type='galactic', mode='edges')
omega = -np.diff(xsky, axis=1)[1:, :] * np.diff(ysky, axis=0)[:, 1:]
Copy link
Member

@adonath adonath Apr 25, 2016

Choose a reason for hiding this comment

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

Can you add a np.abs() call here and remove the minus sign?

Loading

Copy link
Member

@cdeil cdeil Apr 25, 2016

Choose a reason for hiding this comment

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

Just to confirm what @adonath is suggesting: just call np.abs to make it work for all images. Some images will have positive or negative values for CDELT and the easiest way to handle all images is to take the abs value.

Loading

@cdeil
Copy link
Member

@cdeil cdeil commented Apr 25, 2016

(I'm unsubscribing from this PR now, leaving the review to @adonath. If there's something where you want my input, please ping me directly.)

Loading

@@ -285,7 +285,7 @@ def make_exposure(self, obs_id, spectral_index=2.3, for_integral_flux=False):
xpix_coord_grid, ypix_coord_grid = exposure.coordinates(coord_type='pix')
# calculate pixel offset from center (in world coordinates)
coord = pixel_to_skycoord(xpix_coord_grid, ypix_coord_grid, exposure.wcs, origin=0)
center = SkyCoord.from_pixel(self.header["NAXIS1"] / 2., self.header["NAXIS2"] / 2., exposure.wcs)
Copy link
Member

@adonath adonath Apr 25, 2016

Choose a reason for hiding this comment

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

Note, that the definition of SkyMap.center() might be slightly different (see here) from the one used here. Is this covered by a test somehow? If not, could you add at least one assert to the test below? E.g. check that the center coordinates are right...because it might affect science results.

Loading

@adonath
Copy link
Member

@adonath adonath commented Apr 25, 2016

@leajouvin Thanks for the update of ImageAnalysis to the changes I've made to SkyMap. I've left a few minor comments. The only major point I got is, we should make sure, that the definition of the center of a SkyMap is technically correct. But I'll open a separate issue for this.

Right now test_image_pipe.py fails on Travis-CI. Does it pass locally on your machine?

Loading

self.skymap = SkyMap.empty(nxpix=250, nypix=250, binsz=0.02, xref=center.l.deg,
yref=center.b.deg, proj='TAN', coordsys='GAL')

def test_center(self):
Copy link
Author

@JouvinLea JouvinLea Apr 25, 2016

Choose a reason for hiding this comment

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

@adonath
add this test for the center by creating an image centered on the Crab position:

In [17]: SkyCoord.from_name("Crab").galactic
Out[17]: 
<SkyCoord (Galactic): (l, b) in deg
    (184.55745788, -5.78435671)>

When you use the method center of the skymap it gives:

<SkyCoord (Galactic): (l, b) in deg
    (184.55974014, -5.78918015)>

when you use the line I wrote before:
center = SkyCoord.from_pixel(self.header["NAXIS1"] / 2., self.header["NAXIS2"] / 2., exposure.wcs)
it gives:

<SkyCoord (Galactic): (l, b) in deg
    (184.54968906, -5.77918006)>

So the method center is fine no?

Loading

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Apr 25, 2016

@adonath
I took your suggestions into account!!
Travis failed also locally but not for the test I added... Since I didn't code the other tests I don't know why they failed and I didn't want to touch them just to have the good value...

Loading

Lea Jouvin added 2 commits Apr 25, 2016




Copy link
Member

@adonath adonath Apr 25, 2016

Choose a reason for hiding this comment

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

But this test requires gammapy-extra, right? So just add @requires_data('gammapy-extra'). This might fix the Travis-CI build as well.

Loading

@@ -285,7 +285,8 @@ def make_exposure(self, obs_id, spectral_index=2.3, for_integral_flux=False):
xpix_coord_grid, ypix_coord_grid = exposure.coordinates(coord_type='pix')
# calculate pixel offset from center (in world coordinates)
coord = pixel_to_skycoord(xpix_coord_grid, ypix_coord_grid, exposure.wcs, origin=0)
center = SkyCoord.from_pixel(self.header["NAXIS1"] / 2., self.header["NAXIS2"] / 2., exposure.wcs)
center = exposure.center()
Copy link
Author

@JouvinLea JouvinLea Apr 26, 2016

Choose a reason for hiding this comment

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

@adonath
Do you agree now with the test I added that the center() method of the Skymap is ok here?

Loading

Copy link
Member

@adonath adonath Apr 26, 2016

Choose a reason for hiding this comment

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

Yes, looks fine to me!

Loading

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Apr 26, 2016

@adonath
I don't understand why the test don't pass on travis because it passes locally on my machine...

Loading

@adonath
Copy link
Member

@adonath adonath commented Apr 26, 2016

@JouvinLea I assume it's related to the subprocess calls, but I can't say for sure. Is there a way to avoid these? E.g. by using tmpdir.mkdir() etc.?

@cdeil Any further ideas why the test fails of Travis-CI?

Loading

@joleroi
Copy link
Contributor

@joleroi joleroi commented Apr 26, 2016

You could avoid using subprocess by creating the DataStore from gammapy-extra and copying it to tmpdir with the copy_obs command

ds = DataStore.from_dir($GAMMAPY_EXTRA/etc.)
ds.copy_obs(ds.obs_table, tmpdir)

Loading

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Apr 26, 2016

@adonath
I did what @joleroi proposed!
Now we have an error coming from reproject in skymap in travis

from reproject import reproject_interp, reproject_exact

Loading

@adonath
Copy link
Member

@adonath adonath commented Apr 26, 2016

Great, this already looks much better on Travis-CI! I think you have to add @requires_dependency('reproject') to the test_image_pipe function, because it uses the SkyMap.reproject() method to reproject the exclusion mask. Once this is done it should be fine. Thanks for your work!

Loading

@adonath adonath merged commit d584ccc into gammapy:master Apr 26, 2016
2 checks passed
Loading
@cdeil cdeil changed the title Images Fix image pipe class May 19, 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

4 participants