Fix BoundingBox from_float() upper bounds

[larrybradley]

This is yet another fix for BoundingBox.from_float(). In the case where the upper bound is at the pixel edge (e.g. xmax=2.5), the code was effectively rounding up and then adding 1. In those cases, the bounding box upper bound is too large by one pixel.

In other words, the lower and upper bounds were treating x.5 pixel inputs the same (rounding the same way). But because these represent boundaries, we want them to "round inward".

Example:

from regions.core import PixCoord
from regions.shapes.circle import CirclePixelRegion
center = PixCoord(25.0, 25.0)
reg = CirclePixelRegion(center, 4.5)    # NOTE the half-pixel radius here
mask = reg.to_mask(mode='exact')
plt.imshow(mask)

New:

Old:

[cdeil]

@larrybradley - Thanks!

Some suggestions:

• So this PR is about fixing something right on the edge? Should the behaviour on the edge now be well-defined? Can we state a guarantee or at least the intent for the edge at https://github.com/larrybradley/regions/blob/08d57c8c11c807121cda1055ec86126f760aeaf7/regions/core/bounding_box.py#L87 ? If it's a guarantee, we could add a test for the behaviour?
• Maybe add a code comment explaining the implementation? That could be useful for future developers that will not find the pull requests leading to this implementation, and might be tempted to just use a simpler int() call and not realise why this is needed, i.e. to get edge cases and negative numbers rounded correctly. Above you say "round inward", but I think the floor at the min and ceil at the max is "round outward"?
• Can we please make _from_float public? It's so tricky to get right, and we have similar code in Gammapy for sky map cutout / pasting that I'd like to replace by this. I think as a classmethod from_float here could be fine, but a separate utility function would be fine with me as well.

[larrybradley]

@cdeil Yes, this fixes a literal edge case. It's a guarantee modulo possible float-point errors. If radius=4.5 gets stored as4.50000001 then there's going to be an extra 1 pixel added to each side (top, bottom, left, right) of the bounding box (stored as 4.4999999999 will work as expected). However, I don't think there's anything we can do about that.

I worry a bit about make _from_float public because it behaves somewhat differently from the __init__ function. _from_float adds a 1 to the upper bounds to account for the exclusive upper index. The __init__ function doesn't do this (it assumes you have already done it).

Example:

>>> from regions import BoundingBox
>>> BoundingBox(ixmin=1, ixmax=10, iymin=2, iymax=20)
BoundingBox(ixmin=1, ixmax=10, iymin=2, iymax=20)

>>> BoundingBox._from_float(xmin=1, xmax=10, ymin=2, ymax=20)
BoundingBox(ixmin=1, ixmax=11, iymin=2, iymax=21)

That could be very confusing even though the difference is explained in the documentation. If you want to make it public, perhaps _from_float should be renamed?

[cdeil]

@larrybradley - I see your point.

I'd still like to have it publicly available, because calling private methods from other parts of the package (see https://github.com/astropy/regions/search?utf8=%E2%9C%93&q=_from_float) or other Python packages (like Gammapy) is bad. And if it's private, it doesn't show up in the docs, and I can't link to a docs URL to point it out to people.

Concerning method name (or even relocating to somewhere completely else), I don't have a suggestion. If you agree to the change to make it public, maybe you can come up with something?

[larrybradley]

@cdeil I don't have a good name. @astrofrog?

[cdeil]

One option I was thinking about was to always go via regions.RectanglePixelRegion which does support floats, and then access it's bounding_box to do what _from_float does now.

But I don't actually think it's a good suggestion. RectanglePixelRegion has data members of center and width / height, not min / max of corners, and it can have an angle.

There's several other possible solutions: allow float in bounding box init, and add a method or attribute that goes to int index. Or have a separate class for float bounding boxes. Or mostly keep as-is and try to find a good name.

I don't know which of these options would be best here, or have a suggestion for a good name.

Let's wait and see if someone comes up with a good suggestion.
And if not, maybe BoundingBox.from_float with a good docstring would be acceptable?

[sosey]

What about max_extent instead of _from_float?

[cdeil]

@sosey - Thanks for the suggestion!

Personally I would prefer BoundingBox.from_float over BoundingBox.max_extent because the from_* pattern is common in Python for classmethods to create something and feels familiar, whereas max_extent sounds like a property to me.

@larrybradley - Unless others chime in, could you please make the call here on naming and then merge this?

(I'd like to make the regions 0.2 release)

[larrybradley]

@cdeil I made from_float public.

[bsipocz]

@cdeil - I've merged this as it was all green. Now regions should be ready for 0.2.

[cdeil]

@larrybradley - Thanks!