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 map smooth method #1444

Merged
merged 6 commits into from Jun 28, 2018
Merged

Add map smooth method #1444

merged 6 commits into from Jun 28, 2018

Conversation

@AtreyeeS
Copy link
Member

@AtreyeeS AtreyeeS commented Jun 27, 2018

Added smooth()
removed idx from plot()

Copy link
Member

@cdeil cdeil left a comment

@AtreyeeS - Thanks!

See inline comments.

from scipy.ndimage import convolve
from scipy.stats import gmean

if self.geom.ndim>2:

This comment has been minimized.

@cdeil

cdeil Jun 27, 2018
Member

@adonath added m.geom.is_image a few days ago. Suggest to use if not self.geom.is_image: here.

This comment has been minimized.

@AtreyeeS

AtreyeeS Jun 27, 2018
Author Member

Added the same in plot() also

if isinstance(radius, Quantity):
# use geometric mean if x an y pixel scale differ
val=Angle(np.abs(self.geom.wcs.wcs.cdelt),unit="deg")
radius=gmean(radius/val).value

This comment has been minimized.

@cdeil

cdeil Jun 27, 2018
Member

This pixel scale computation should happen in map.geom, and it shouldn't be the geometric mean.

My suggestion would be to call this here:

if isinstance(radius, Quantity):
    radius = radius.to('deg').value / self.geom.pixel_scales.mean()

and to add this to WCSGeom:

import astropy.wcs.utils
from astropy.coordinates import Angle

def pixel_scales(self):
    """TODO: describe"""
    return Angle(astropy.wcs.utils.proj_plane_pixel_scales(self.wcs), 'deg')

calling http://docs.astropy.org/en/latest/api/astropy.wcs.utils.proj_plane_pixel_scales.html

This will work for WCS that are given by CDELT, but also other WCS given e.g. by CD.

Note that there is already a http://docs.gammapy.org/dev/api/gammapy.maps.WcsGeom.html#gammapy.maps.WcsGeom.pixel_area
that should be done in exactly the same way using
http://docs.astropy.org/en/latest/api/astropy.wcs.utils.proj_plane_pixel_area.html

Considering changing the existing
http://docs.gammapy.org/dev/api/gammapy.maps.WcsGeom.html#gammapy.maps.WcsGeom.width
to be what I propose for pixel_scale, i.e. an Angle instead of a tuple, should be a separate discussion / issue.
But I think making the change to add geom.pixel_scales could be done here, since that is what you should call in map.smooth.

if self.geom.ndim>2:
raise ValueError('Only supported on 2D maps')

image = copy.copy(self)

This comment has been minimized.

@cdeil

cdeil Jun 27, 2018
Member

Can you please put that copy line at the end?
I.e. in the method use local variable data and only make the new Map object at the end.

@@ -531,3 +542,63 @@ def make_region_mask(self, region, inside=True):
np.logical_not(mask, out=mask)
# TODO : update meta table to include something about the region used for mask creation?
return WcsNDMap(geom=self.geom, data=mask, meta=self.meta)


def smooth(self, kernel='gauss', radius=1.0, **kwargs):

This comment has been minimized.

@cdeil

cdeil Jun 27, 2018
Member

I think I'd prefer the signature:

def smooth(self, radius, kernel='gauss', **kwargs)

You had default radius=1.0 pix, but most callers will want to choose the smooth radius, there's no good default value, no?

This comment has been minimized.

@AtreyeeS

AtreyeeS Jun 27, 2018
Author Member

I thought it would be nice to have a default value so that the function does not break in case the user does not know what smoothing to start off with, a smoothing of 1 pixel actually implies no smoothing..
Anyways, I have removed it now!


kwargs.setdefault('interpolation', 'nearest')
kwargs.setdefault('interpolation', 'None')

This comment has been minimized.

@cdeil

cdeil Jun 27, 2018
Member

Why did you change to None?

kwargs.setdefault('interpolation', 'None')

I find None a bit confusing (and there's also none which is different):
https://matplotlib.org/gallery/images_contours_and_fields/interpolation_methods.html
Maybe stick with nearest?

if add_cbar:
cbar = fig.colorbar(caxes, ax=ax)
else:
cbar=None

This comment has been minimized.

@cdeil

cdeil Jun 27, 2018
Member

Could be written in one line:

cbar = fig.colorbar(caxes, ax=ax) if add_cbar else None
AtreyeeS added 2 commits Jun 27, 2018
@AtreyeeS
Copy link
Member Author

@AtreyeeS AtreyeeS commented Jun 27, 2018

Thanks @cdeil !
Made the changes as suggested

@cdeil cdeil self-assigned this Jun 27, 2018
@cdeil cdeil added the feature label Jun 27, 2018
@cdeil cdeil added this to the 0.8 milestone Jun 27, 2018
@cdeil cdeil changed the title Smooth() Add map smooth method Jun 27, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Jun 27, 2018

@AtreyeeS - Thanks.

I'll try this out locally and merge soon.

@cdeil cdeil dismissed their stale review via b0b5cb3 Jun 27, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Jun 27, 2018

@AtreyeeS - Some fixes in b0b5cb3

For future PRs: suggest to run tests locally, e.g. in this case:

python -m pytest -v gammapy/maps -k smooth

That catches most issues, that then only show up later in CI or for your reviewer:
https://ci.appveyor.com/project/cdeil/gammapy/build/1.0.3532/job/vqji7ug6g9ouvh3v#L1500

@adonath - OK to merge?
(I'm off to see the game now, if there's other issues or changes, can you make them directly, please?)

@cdeil cdeil added this to To do in gammapy.maps via automation Jun 27, 2018
@cdeil cdeil assigned adonath and unassigned cdeil Jun 27, 2018
@adonath adonath self-requested a review Jun 27, 2018
@cdeil
cdeil approved these changes Jun 28, 2018
Copy link
Member

@adonath adonath left a comment

Looks good to me now! The docs build is gives a warning, but I'll fix that in #1443. So I'll go ahead and merge this PR now. Thanks, @AtreyeeS!

@adonath adonath merged commit 5fb39d2 into gammapy:master Jun 28, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
gammapy.maps automation moved this from To do to Done Jun 28, 2018
@cdeil cdeil mentioned this pull request Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.maps
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants