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 function #167

Merged
merged 2 commits into from Aug 21, 2014

Conversation

Projects
None yet
3 participants
@ellisowen
Contributor

ellisowen commented Aug 15, 2014

Made a pull request while I work on this in case you have comments.

@ellisowen

This comment has been minimized.

Contributor

ellisowen commented Aug 17, 2014

OK, this is also now ready for review.
As before, the Travis CI failures don't relate to anything in this PR as far as I can tell.

@coveralls

This comment has been minimized.

coveralls commented Aug 18, 2014

Coverage Status

Coverage increased (+0.64%) when pulling 6e1b0af on ellisowen:profiles into f30420f on gammapy:master.

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 18, 2014

  • I think having this functionality would be nice, but we'll have to iterate on the API and implementation a bit ... I'll leave inline comments.
  • Yes, the travis-ci test failures are unrelated and can be ignored.
@cdeil

This comment has been minimized.

Member

cdeil commented Aug 20, 2014

It would have been nicer to make the FluxProfile class work properly (and implement this as a special case). But I realise you don't have much time left, so OK to merge this as a temp solution and I'll refactor it later. I'll leave some inline comments, please apply the equivalent fixes to #166.

Counts image to allow Poisson errors to be calculated. If not provided,
a standard_error should be provided, or zero errors will be returned.
(Optional).
mask_array : array_like

This comment has been minimized.

@cdeil

cdeil Aug 20, 2014

Member

mask_array -> mask

(Optional).
mask_array : array_like
2D mask array, matching spatial dimensions of input image. (Optional).
invert : bool

This comment has been minimized.

@cdeil

cdeil Aug 20, 2014

Member

Remove the invert option.
Instead leave this step up to the user before calling this functions.

Use the Astropy mask convention stated here:

A mask value of True indicates a value that should be ignored, while a mask value of False indicates a valid value.

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 20, 2014

The code for the _lat_ and _lon_ function is almost identical ... can't you just use an argument profile_axis='lon' or profile_axis='lat' and then avoid duplicating the code somehow?
(e.g. by putting the code that is now different in the two functions into an if statement)

Optional: It also would have been much nicer if you reused and expanded as needed the compute_total_stats and / or FluxProfile.compute method ... the statistics of computing e.g. the flux and flux error should be always the same irrespective of how the contributing pixels are distributed (and there's the two methods that you wanted to study).

@ellisowen ellisowen force-pushed the ellisowen:profiles branch from 6e1b0af to ce3b0ff Aug 20, 2014

@ellisowen

This comment has been minimized.

Contributor

ellisowen commented Aug 20, 2014

  • The code for the lat and lon function is almost identical ... can't you just use an argument profile_axis='lon' or profile_axis='lat' and then avoid duplicating the code somehow?
    (e.g. by putting the code that is now different in the two functions into an if statement)

Agreed - done!

  • Optional: It also would have been much nicer if you reused and expanded as needed the compute_total_stats and / or FluxProfile.compute method ... the statistics of computing e.g. the flux and flux error should be always the same irrespective of how the contributing pixels are distributed

I agree - I think I will aim to do this in a new PR maybe next week or the week after.

  • (and there's the two methods that you wanted to study).

This works OK already to do this - I can just pass in counts & exposure image to get two profiles to divide, or passes in a flux image for the other method.

@ellisowen ellisowen force-pushed the ellisowen:profiles branch from ce3b0ff to ef9e9a0 Aug 20, 2014

@ellisowen

This comment has been minimized.

Contributor

ellisowen commented Aug 21, 2014

OK, this should now address your comments.

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 21, 2014

OK ... merging this as a temp solution (until someone has time to work on FluxProfile and put this as a special case).

Thanks!

cdeil added a commit that referenced this pull request Aug 21, 2014

@cdeil cdeil merged commit 8cf077e into gammapy:master Aug 21, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@cdeil cdeil changed the title from Profiles to Add image profile function Apr 8, 2015

@cdeil cdeil added the feature label Apr 8, 2015

@cdeil cdeil added this to the 0.1 milestone Apr 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment