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 image profile class #833

Merged
merged 9 commits into from Jan 9, 2017
Merged

Conversation

@adonath
Copy link
Member

@adonath adonath commented Jan 7, 2017

This PR implements a generic ImageProfile class, which offers plotting, smoothing and renormalization methods. The main data structure is an astropy table, which stores the coordinate values x_ref or x_min and x_max and the profile data values profile and profile_err.

@adonath adonath added the feature label Jan 7, 2017
@adonath adonath added this to the 0.6 milestone Jan 7, 2017
@adonath adonath self-assigned this Jan 7, 2017
@cdeil
Copy link
Member

@cdeil cdeil commented Jan 8, 2017

👍

This is almost independent of image and angles, so this could be changed to a more general "Profile1D" class. Not sure, maybe keep as-is for now?

Copy link
Member

@cdeil cdeil left a comment

I've left a bunch of inline comments.

Only one is really a bug (profile -> profile_err copy & paste error), the rest is up to debate (do what you think best for now, no need for discussion / further review).

Adding one example in the docstring of high-level docs to advertise this would be great!

"""
Image profile class.
The image profile data is stored in `~astropy.table.Table` object, with the

This comment has been minimized.

@cdeil

cdeil Jan 8, 2017
Member

I'd suggest to put a Parameters section, even if there's just one parameter in __init__.
Keeping the table column description in the docstring is fine, I think.

This comment has been minimized.

@cdeil

cdeil Jan 8, 2017
Member

Does this work for tables with columns that don't have units?
Or are there certain requirements on units for certain columns?

(my suggestion would be to not hard-code ImageProfile specific behaviour, and make this more of a general Profile1D class)

This comment has been minimized.

@adonath

adonath Jan 9, 2017
Author Member

I agree there's nothing special about this class, that would restrict the use to image based profile measurements. For now I'd like to keep it as is, but let's keep in mind the class could be used for acceptance or other profiles as well...

x_j = \sum_i x_{(j - i)} h_i
Where :math:h_i are coefficients of the convolution kernel.

This comment has been minimized.

@cdeil

cdeil Jan 8, 2017
Member

Does this work? I think backticks are needed, no?

:math:`h_i`

Is it really useful to give the general formula how 1D convolution works?
Why not just say "convolution"?

radius : `~astropy.units.Quantity` or float
Smoothing width given as quantity or float. If a float is given it
interpreted as smoothing width in pixels. If an (angular) quantity
is given it converted to pixels.

This comment has been minimized.

@cdeil

cdeil Jan 8, 2017
Member

If a float is given it interpreted as smoothing width in pixels.

I don't think that's true, you always execute:

radius = np.abs(radius / np.diff(self.x_ref))[0]
width = 2 * radius.value + 1

I think I would prefer to only support float width in pix here, because smoothing is done in pix.
If you keep the quantity width, you should document that width = self.x_ref[1] - self.x_ref[0] is used to convert to float.

Looking at http://docs.gammapy.org/en/latest/api/gammapy.image.SkyImage.html#gammapy.image.SkyImage.smooth I see that the links to uniform_filter and gaussian_filter in scipy aren't working.
Probably it has to be ~scipy.ndimage.uniform_filter and ~scipy.ndimage.gaussian_filter?
Can you please fix this there and here?

This comment has been minimized.

@adonath

adonath Jan 9, 2017
Author Member

Done

"""
Image profile error quantity.
"""
return self.table['profile'].quantity

This comment has been minimized.

@cdeil

cdeil Jan 8, 2017
Member

profile -> profile_err

Add a test that catches this?

Parameters
----------
**kwargs : dict
Keyword arguments passed to plt.plot()

This comment has been minimized.

@cdeil

cdeil Jan 8, 2017
Member

Link to plot function in MPL, so that people can quickly look up the valid parameters?

This comment has been minimized.

@adonath

adonath Jan 9, 2017
Author Member

Done



@requires_dependency('scipy')
def test_smooth(self):

This comment has been minimized.

@cdeil

cdeil Jan 8, 2017
Member

Add some more tests and asserts for smooth?
E.g. Gauss smooth is never executed I think -> does it work?
Does profile_err get filled with reasonable values?

@adonath
Copy link
Member Author

@adonath adonath commented Jan 9, 2017

@cdeil Thanks for the review, I've addressed your comments with the latest commits. I'll merge once Travis-CI passed.

@adonath adonath merged commit 49f8b97 into gammapy:master Jan 9, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@adonath
Copy link
Member Author

@adonath adonath commented Jan 9, 2017

Travis fails were unrelated.

@adonath adonath deleted the adonath:add_image_profile_class branch Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants