Remove SkyMask (merge with SkyImage) #966
Conversation
| in different applications (or link to other docs). | ||
| """ | ||
|
|
||
| def fill_random_circles(self, n=4, min_rad=0, max_rad=40): |
There was a problem hiding this comment.
@cdeil
It seems like you have deleted this method. I would like to continue using it. Can we add it back?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
PS: there was one single caller for fill_random_circles in the docs, and I did update that example.
There was a problem hiding this comment.
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
👍
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.datarepresents a mask (i.e. only has values 0 and 1) at the start.Maybe add a helper method
_check_is_maskthat 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.readfirst and then come back to this PR. #539 (comment)