Implement to_mask for circular, elliptical, and rectangular regions

[astrofrog]

This is a WIP that replaces/re-uses some of #17. Not at all ready for review/comments.

[astrofrog]

@larrybradley - just FYI, I added a simple Mask class here I wrote during my flight, but just replace it with yours (I think regions.core.mask is the right place though)

[larrybradley]

@astrofrog Did you allow for maintainers to edit? I tried to push to your branch, but it's not letting me. I'll just submit a PR to your branch....

[astrofrog]

@larrybradley - I had that issue before and realized it's because I'd set up a read-only remote. Anyway, I'll merge the PR, so no worries :) Thanks!

[astrofrog]

@larrybradley - for Mask, I was thinking, maybe it's enough to have origin instead of having bbox_slice - the origin should be enough information to construct the slice, right?

[astrofrog]

@larrybradley - see e.g. 50a57eb

[larrybradley]

@astrofrog Yes, that works too. For photutils, I simply passed the tuple of slices because I already have them defined for each aperture/position (e.g. see https://github.com/astropy/photutils/blob/master/photutils/aperture/circle.py#L148) instead of recalculating them.

So instead you would simply pass the origin as the equivalent of myslice[0].start, slice[1].start?

[larrybradley]

Here's an example of the aperture to_mask() method from the CircularMaskMixin class:
https://github.com/astropy/photutils/blob/master/photutils/aperture/circle.py#L29

[astrofrog]

So instead you would simply pass the origin as the equivalent of myslice[0].start, slice[1].start?

Yes, exactly - the extent of the slice is redundant with the size of the mask (and were indeed checking for consistency before)

[astrofrog]

Almost done... Need tests for Mask!

[astrofrog]

This is ready for reviewing. What this does:

• Implements to_mask for circular, elliptical, and rectangular regions (the only missing mode is exact mode for rectangular regions)
• Adds reference tests using pytest-fits to compare the masks to reference versions stored in FITS files (fixes Have a standardized way of testing correctness #23)
• Adds a Mask class (fixes Return a Mask class from the to_mask() method #55)
• Remove 'any' and 'all' modes (fixes Remove 'all' and 'any' models from to_mask API? #72)

This doesn't include tests for the Mask class yet, but @larrybradley told me he'd add them in a separate pull request.

Comments welcome!

[cdeil]

@astrofrog - Thanks!

@larrybradley or @bsipocz - Do you have time to review this PR?

@astrofrog - What about high-level docs? Can you add them in this PR or do you prefer to make a reminder issue that this should be done in a follow-up PR?

E.g. this section on masks should be updated (currently points to photutils):
http://astropy-regions.readthedocs.io/en/latest/getting_started.html#masks

[astrofrog]

@cdeil @bsipocz @larrybradley - I've added documentation, which you can also read here:

http://astropy-regions-astrofrog.readthedocs.io/en/latest/getting_started.html#masks

So this is ready for review!

[cdeil]

This docstring says that a mask and slices are returned:
http://astropy-regions-astrofrog.readthedocs.io/en/latest/api/regions.PixelRegion.html#regions.PixelRegion.to_mask

I think that's not the case, only a mask object is returned as shown in the examples:
http://astropy-regions-astrofrog.readthedocs.io/en/latest/getting_started.html#masks

Another thing I noticed is that the to_mask methods don't do input validation.
I.e. writing mode='centre' instead of mode='center' could error in a weird way or silently do something different from what the user wanted to do.
Maybe add input validation for such multiple-choice string options?

@staticmethod
def validate_mode(mode):
    if mode not in set('spam', 'ham', ...):
        raise ValueError(...)

[cdeil]

The Mask docstring doesn't have any examples:
http://astropy-regions-astrofrog.readthedocs.io/en/latest/api/regions.Mask.html

Can you link from it to the high-level docs mask section, which has several examples?

The docs show regions.Mask(mask, origin), but the docstring documents parameters mask and bbox_slice!?
Also, please add a one-line docstring for the slices attribute.
Maybe make the __init__ arguments and data members of the class the same (if that makes sense, I didn't look closely).

[astrofrog]

@cdeil - I think I've implemented all your comments (with help from @larrybradley)

[cdeil]

@astrofrog - Thanks!

I had a quick look at the high-level docs with @adonath.

Some more feedback:

• The array and slices attributes should have a one-line description here
• For origin I'd suggest you use a PixCoord object. The idea is that we use it everywhere, and not tuples in some places. The main advantage is that it's harder to screw up the x, y order. Suggesting to remove it is a valid proposal, but given that we have it at the moment, I think we should use it.
• If I understand correctly, the pix_region.to_mask method does two things: first compute minimal bounding box (including large image edge handling), then compute the mask array. My preference would be to introduce a simple small BoundingBox class (similar to PixCoord), that holds the two slices and again knows about axis order and has a nice repr. And maybe also expose the two computation steps separately, i.e. make it possible to just compute the bounding box for a given region, without computing the mask array?
• The high-level docs example never shows how the mask relates to the large original image. I would suggest ending the high-level docs section either with an example for that, or to end like this "this is a building block for aperture photometry. How to actually use it will be shows in photutils once that package has been updated to use regions". That's the plan, no?

Just FYI: What you've built here is a bit similar (but really pretty different) to what @adonath implemented for gammapy.image.SkyImage:
http://docs.gammapy.org/en/latest/image/sky_image.html#cutout-and-paste

[cdeil]

So this copies over ~ 1000 lines of code from photutils, right?

When you merge it, could you please open a new issue in the regions or photutils tracker describing the next steps and plan to continue with this? I guess keep adding things to regions for a few months, and then in a year or so rewrite photutils.aperture to use that?

[astrofrog]

@cdeil - thanks for the comments! Will work on this on the ✈️ tonight.

So this copies over ~ 1000 lines of code from photutils, right?

Yes, the Cython code is copied over.

When you merge it, could you please open a new issue in the regions or photutils tracker describing the next steps and plan to continue with this? I guess keep adding things to regions for a few months, and then in a year or so rewrite photutils.aperture to use that?

Will do - from talking to @larrybradley, that is indeed the plan.

[astrofrog]

The array and slices attributes should have a one-line description here

These are no longer public methods, so not needed

For origin I'd suggest you use a PixCoord object. The idea is that we use it everywhere, and not tuples in some places. The main advantage is that it's harder to screw up the x, y order. Suggesting to remove it is a valid proposal, but given that we have it at the moment, I think we should use it.

Good idea - though as I started to implement it I realized that here we specifically want integer pixel coordinates, but PixCoord allows floating-point coordinates. Do we want to have a separate class for integer coordinates? If not, should we just check in Mask.init that the PixCoord has integer values?

[astrofrog]

@cdeil - I cleaned the FITS files out of the history, so I'll go ahead and merge now!