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 ExclusionMask.distance #631

Merged
merged 2 commits into from Jul 20, 2016
Merged

Conversation

@OlgaVorokh
Copy link
Member

@OlgaVorokh OlgaVorokh commented Jul 16, 2016

@cdeil I create this PR based on this issue.

According your comment in the issue I should do this things in the PR:

  • Start by adding a test to reproduce the issue.
  • Then move the code from the standalone function here inside the ExclusionMask.distance_image method.
  • Investigate and fix the issue ... maybe a manual change in dtype is needed to get it to work?
@OlgaVorokh
Copy link
Member Author

@OlgaVorokh OlgaVorokh commented Jul 17, 2016

@cdeil Can you review this?

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

@cdeil cdeil commented Jul 18, 2016

https://travis-ci.org/gammapy/gammapy/jobs/145330433#L1509
You need to add a @requires_dependency('scipy') to this test.

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 18, 2016

Please update the task list of this PR.
I often replace task lists with a description "This PR does ..." once it's ready for final review or to be merged.

Returns
-------
distance : `~numpy.ndarray`

This comment has been minimized.

@cdeil

cdeil Jul 18, 2016
Member

Should we return SkyMap or numpy.array for such methods?
I think maybe SkyMap would be more convenient, but either way is fine with me.

This comment has been minimized.

@adonath

adonath Jul 18, 2016
Member

I don't care either. I think the distance image is mostly used internally in some algorithms. But it would be definitely useful to have the distance as an Angle or Quantity object with physical units attached.

This comment has been minimized.

@cdeil

cdeil Jul 18, 2016
Member

We decided to return a SkyMap in the GSoC call just now.

My preference would be to implement #632 and then add an option here that returns the sky map in degree, where the pixel distance image is multiplied by proj_plane_pixel_scales. But that's for a future PR, we don't have to discuss here.

def test_distance_image():
pass
# TODO
mask = ExclusionMask.empty(nxpix=300, nypix=200)

This comment has been minimized.

@cdeil

cdeil Jul 18, 2016
Member

Please make the test image smaller, so that the result can easily be printed and looked at.

It would also be good to have three input test images for this:

  • Everything excluded
  • Nothing excluded
  • Some pixels excluded, maybe an edge and center pixel or block of few pixels.

This comment has been minimized.

@cdeil

cdeil Jul 18, 2016
Member

As discussed in the GSOC call just now: it's not clear what the behaviour should be for input masks that are all or nothing excluded. @OlgaVorokh - Just add tests that establish the current behaviour for now, and then we discuss what we want.

I think maybe if nothing is excluded, the distance should be very large. We could put it to the max positive float.
And if everything is excluded, that's like being deep inside an exclusion regions. We could put the min negative float.
That behaviour should be most convenient when working with distance images in algorithms because it does the right thing (as opposed to having NaN and needing to special-case those cases in every algorithm.

@OlgaVorokh
Copy link
Member Author

@OlgaVorokh OlgaVorokh commented Jul 19, 2016

@cdeil Can you review this?

@@ -19,6 +17,7 @@
'make_tevcat_exclusion_mask'
]

MAX_VALUE = 1e10

This comment has been minimized.

@cdeil

cdeil Jul 20, 2016
Member

I'd suggest to move this MAX_VALUE inside the distance_image method.
This is closer to where it's used and thus code that's easier to read / maintain.
If we ever need it somewhere else (probably never) we can then make it global.

and then combine both distance images into one, using negative
distances (note the minus sign) for pixels inside exclusion regions.
If data consist only of ones, it'll be supposed to be far away from zero pixsels,

This comment has been minimized.

@cdeil

cdeil Jul 20, 2016
Member

Typo: pixsels -> pixels

distance : `~gammapy.image.SkyMap`
Sky map of distance to nearest exclusion region.
"""
if np.all(self.mask == 1):

This comment has been minimized.

@cdeil

cdeil Jul 20, 2016
Member

I know it's a matter of taste, but I would suggest to have the special case return the special result, and then to have the normal case not be indented in an else block.

I.e. use this:

if np.all(self.mask == 1):
    return SkyMap.empty_like(self, fill=MAX_VALUE)

# continue here with what you have in the else block, but not indented.
pass
# TODO
mask = ExclusionMask.empty(nxpix=3, nypix=2)
skymap = mask.distance_image

This comment has been minimized.

@cdeil

cdeil Jul 20, 2016
Member

Combine these two lines:

skymap = mask.distance_image
distance = skymap.data

into one

distance = mask.distance_image.data

here and also below.

assert_allclose(distance[-1, -1], 1e10)

data = np.ones((2, 3))
data[0, :2] = 0.

This comment has been minimized.

@cdeil

cdeil Jul 20, 2016
Member

Replace these two lines:

data = np.ones((2, 3))
data[0, :2] = 0.

with this one line:

data = [[0, 0, 1], [1, 1, 1]]

One less line and easier to read (at least for me).

assert_allclose(distance[0, 0], -1.)
assert_allclose(distance[-1, 0], 1.)
assert_allclose(distance[0, -1], 1.)
assert_allclose(distance[-1, -1], 1.41421356)

This comment has been minimized.

@cdeil

cdeil Jul 20, 2016
Member

Add newline at end of file.

This comment has been minimized.

@cdeil

cdeil Jul 20, 2016
Member

Again, because the test matrix is so small, you could just put this, which is less lines and IMO a little more readable.

expected = [[-1, -1, 1], [1, 1, 1.41421356]]
assert_allclose(distance, expected)
assert_allclose(distance[0, 0], -1.)
assert_allclose(distance[-1, 0], -2.)
assert_allclose(distance[0, -1], -2.23606798)
assert_allclose(distance[-1, -1], -2.82842712)

This comment has been minimized.

@cdeil

cdeil Jul 20, 2016
Member

The results for input of mask of all zeros is still weird.
It measures the distance to some edge or corner, which never makes sense for our applications.

Can you also check for that and in that case return -1e10?
This has the meaning "we're assuming we are deep inside an exclusion region".

OK?

This comment has been minimized.

@OlgaVorokh

OlgaVorokh Jul 20, 2016
Author Member

Yes, I agree with you, OK


mask = ExclusionMask.empty(nxpix=3, nypix=2, fill=1.)
distance = mask.distance_image.data
expected = [[1e10, 1e10, 1e10], [1e10, 1e10, 1e10]]

This comment has been minimized.

@cdeil

cdeil Jul 20, 2016
Member

For constant arrays, there's no need to duplicate the values, you can just compare array to scalar and Numpy broadcasting will take care of comparing every value:

assert_allclose(distance, 1e10)
@OlgaVorokh OlgaVorokh force-pushed the OlgaVorokh:exclusionmask_cleanup branch from 1ede43b to 49d8237 Jul 20, 2016
OlgaVorokh added 2 commits Jul 16, 2016
…_image, add test for ExclusionMask.distance_image
…ests for it, add two exceprional situations in method and fix the docs
@OlgaVorokh OlgaVorokh force-pushed the OlgaVorokh:exclusionmask_cleanup branch from 49d8237 to 5291fb7 Jul 20, 2016
@OlgaVorokh
Copy link
Member Author

@OlgaVorokh OlgaVorokh commented Jul 20, 2016

@cdeil Can you review this?

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 20, 2016

Perfect. Thanks!

@cdeil cdeil merged commit f3398d1 into gammapy:master Jul 20, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil cdeil mentioned this pull request Jul 27, 2016
0 of 3 tasks complete
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

3 participants