Round to int, not float

[cdeil]

@astrofrog added bounding box methods that call round:
https://github.com/astropy/regions/search?utf8=%E2%9C%93&q=round

Now here's what I think is an amazing catch by PyCharm (or a false positive if I misunderstand the situation): It points out that e.g. CirclePixelRegion.bounding_box calls BoundingBox(ixmin, ixmax, iymin, iymax) with floats, but it should call it with ints.

Now first of all, I think it knows about the expected types because it's parsing the BoundingBox Numpy-format docstring. Also, it asked me if this project should be simultaneously Python 2.7 and 3 compatible and I said yes. And lastly, on Python 2, round returns float:

$ python2
>>> type(round(0.3))
<type 'float'>

$ python3
>>> type(round(0.3))
<class 'int'>

@astrofrog @adonath - Is this indeed a potential issue? I think we should change callers to always call int, instead of auto-casting of ints stored in float in BoundingBox.__init__? Should we add an extra check that ints are passed in? What's the best (correct, short) way to write this, i.e. what code changes should we make?

Should we do a general review of round code in astropy, photutils, gammapy or just make a change here? Does one of you have time to make a PR or should I?

[astrofrog]

Nice catch!

I think we should change callers to always call int, instead of auto-casting of ints stored in float in BoundingBox.init?

Yes

Should we add an extra check that ints are passed in? What's the best (correct, short) way to write this, i.e. what code changes should we make?

I think just simply doing the pragmatic:

import numbers
if not isinstance(ixmin, numbers.Integral) or not isinstance(...
    raise TypeError('...')

would be fine (works with int as well as numpy integer types).

[astrofrog]

Should we do a general review of round code in astropy, photutils, gammapy or just make a change here? Does one of you have time to make a PR or should I?

Please feel free to do a PR to fix it in regions. A code review of the other packages will probably have to wait a bit since it might take some time.

[cdeil]

I'll make a PR for regions now.
And maybe have a look at other Astropy packages as well.

[bsipocz]

Nice catch @cdeil.
Maybe for photutils we can open an issue to collect post regions v0.2 todos, and list this in there (unless you prefer to make the fixes now).

[cdeil]

I've been thinking about this a bit. My suggestion would be to add a classmethod BoundingBox.from_float (or some better name if someone suggests one).

Currently the float -> int conversion is 3 times the same for circle, rectangle and ellipse,
and one could improve and centralise this in BoundingBox.from_float and call it three times.

@astrofrog - OK?

[astrofrog]

I think we need to be careful here - is the purpose of from_float to return the minimal bounding box that could contain a rectangle in floating point coordinates? Or to round things that should be integers but are float like 3.0? We should make sure this is explained well - I think the former could actually be the most useful to reduce code duplication, but not 100% sure.

[cdeil]

@astrofrog - I've attached a commit here, please have a look.

For now, I've made BoundingBox._from_float private, and mainly added it to avoid the triple code duplication and fix the int(round()) issue in one place.

If someone has a suggestion for name / options / semantics, I could implement that here and we could make it public.

In any case, I plan to add a regression test here and maybe also the if not isinstance(ixmin, numbers.Integral) input validation.

[cdeil]

I've added a better docstring and tests.

Concerning int type validation, I'll file a separate issue, I don't want to do it in this PR.

[larrybradley]

I was just reviewing this. I'll submit a new PR.

[larrybradley]

For the record, photutils does not use np.round anywhere (and that's a good thing!).