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

Remove SkyMask (merge with SkyImage) #966

Merged
merged 3 commits into from Apr 11, 2017

Conversation

Projects
None yet
2 participants
@cdeil
Member

cdeil commented Apr 10, 2017

This pull request removes SkyMask, instead using SkyImage everywhere.

The reason is that we're now getting more image and cube classes and I think that a separate hierarchy of corresponding mask classes doesn't make sense. Generally I'm a fan of having separate classes for separate things, but in this case I don't see the advantage of splitting masks out. At the moment there's one useful property "distance_image" which I'm moving from SkyImage to SkyMask, and even if there are a few more, I think it'll be OK to just check that self.data represents a mask (i.e. only has values 0 and 1) at the start.

Maybe add a helper method _check_is_mask that raises a ValueError if it's not?

Tests don't pass here at the moment. I'll make a PR to fix this issue with SkyImage.read first and then come back to this PR. #539 (comment)

@cdeil cdeil added the cleanup label Apr 10, 2017

@cdeil cdeil added this to the 0.6 milestone Apr 10, 2017

@cdeil cdeil self-assigned this Apr 10, 2017

@cdeil cdeil referenced this pull request Apr 10, 2017

Merged

Introduce smart FITS HDU list class and use for SkyImage #967

3 of 3 tasks complete

@cdeil cdeil merged commit 481e22c into gammapy:master Apr 11, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
in different applications (or link to other docs).
"""
def fill_random_circles(self, n=4, min_rad=0, max_rad=40):

This comment has been minimized.

@joleroi

joleroi Apr 13, 2017

Contributor

@cdeil
It seems like you have deleted this method. I would like to continue using it. Can we add it back?

@joleroi

joleroi Apr 13, 2017

Contributor

@cdeil
It seems like you have deleted this method. I would like to continue using it. Can we add it back?

This comment has been minimized.

@cdeil

cdeil Apr 13, 2017

Member

I don't think it's a good idea to bring this back. It's just something random and complicated, where in tests or examples something non-random and simpler (like a circle or box) would be better.

If you must, please put it somewhere where it's not visible to users (e.g. if it's in one of the test py files you can still import it from everywhere).

@cdeil

cdeil Apr 13, 2017

Member

I don't think it's a good idea to bring this back. It's just something random and complicated, where in tests or examples something non-random and simpler (like a circle or box) would be better.

If you must, please put it somewhere where it's not visible to users (e.g. if it's in one of the test py files you can still import it from everywhere).

This comment has been minimized.

@joleroi

joleroi Apr 13, 2017

Contributor

The problem is that all docs example are quite boring now because the exclusion masks have been replace with empty maps. It's easy to fix that by putting some circles or so by hand, but I have to know that this has changed in order to do that.

@joleroi

joleroi Apr 13, 2017

Contributor

The problem is that all docs example are quite boring now because the exclusion masks have been replace with empty maps. It's easy to fix that by putting some circles or so by hand, but I have to know that this has changed in order to do that.

This comment has been minimized.

@cdeil

cdeil Apr 13, 2017

Member

IMO we should do this for docs examples:

  • remove all traces of TeVCat.
  • Introduce a new helper function that makes circle sky regions for gamma-cat
  • Use that for docs examples

Alternatively, showing users how they can create their own regions to cut out e.g. a box or circle or polygon for the Galactic center is IMO better. A function in Gammapy that makes random circles doesn't really help users, no?

@cdeil

cdeil Apr 13, 2017

Member

IMO we should do this for docs examples:

  • remove all traces of TeVCat.
  • Introduce a new helper function that makes circle sky regions for gamma-cat
  • Use that for docs examples

Alternatively, showing users how they can create their own regions to cut out e.g. a box or circle or polygon for the Galactic center is IMO better. A function in Gammapy that makes random circles doesn't really help users, no?

This comment has been minimized.

@cdeil

cdeil Apr 13, 2017

Member

PS: there was one single caller for fill_random_circles in the docs, and I did update that example.

@cdeil

cdeil Apr 13, 2017

Member

PS: there was one single caller for fill_random_circles in the docs, and I did update that example.

This comment has been minimized.

@joleroi

joleroi Apr 13, 2017

Contributor

IMO we should do this for docs examples:

remove all traces of TeVCat.
Introduce a new helper function that makes circle sky regions for gamma-cat
Use that for docs examples

👍

@joleroi

joleroi Apr 13, 2017

Contributor

IMO we should do this for docs examples:

remove all traces of TeVCat.
Introduce a new helper function that makes circle sky regions for gamma-cat
Use that for docs examples

👍

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