Skip to content
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

Fix bounding box "rounding" #84

Merged
merged 3 commits into from
Nov 28, 2016
Merged

Conversation

larrybradley
Copy link
Member

@larrybradley larrybradley commented Nov 28, 2016

This PR is a followup to #81, which used the round function. We don't want to use round to compute the bounding-box edges because of its behavior at half-pixel values. For such cases round (in Python 3) rounds to the nearest even number, e.g. round(0.5) = 0.0 but round(1.5) = 2.0. That behavior may make sense in a statistical sense, but it's incorrect here for computing bounding box edges.

In this PR, I use what I implemented in my refactor of photutils.aperture:

imin = np.floor(value + 0.5).astype(int)
imax = np.floor(value + 1.5).astype(int)

which "rounds" consistently for the bounding box edges.

Also note that the above behavior changed for Python 3. In Python 2, round would always round away from zero, e.g. round(0.5) = 1.0 and round(1.5) = 2.0. This is yet another inconsistency that we want to avoid (and another reason why round should not be used).

@larrybradley larrybradley added this to the 0.2 milestone Nov 28, 2016
@cdeil cdeil self-assigned this Nov 28, 2016
@cdeil cdeil added the bug label Nov 28, 2016
@cdeil
Copy link
Member

cdeil commented Nov 28, 2016

@larrybradley - Thanks!

You put this:

ixmin = np.floor(xmin + 0.5).astype(int)

Is this the same?

ixmin = int(xmin + 0.5)

If yes, I think I would slightly prefer it.
But in any case, just a thought ... we'll leave what you think best.

Another question: should we really have a test for 0.5?
It's a fractional float. Is it really guaranteed to round the way you have in the test on all machines?

@larrybradley
Copy link
Member Author

You put this:

ixmin = np.floor(xmin + 0.5).astype(int)
Is this the same?

ixmin = int(xmin + 0.5)
If yes, I think I would slightly prefer it.
But in any case, just a thought ... we'll leave what you think best.

For non-negative xmin they are the same, but they can differ for negative xmin, e.g.

>>> xmin = -1
>>> np.floor(xmin + 0.5).astype(int)
-1

>>> int(xmin + 0.5)
0

Negative values in the bounding boxes can occur if the region is only partially on the image. The Mask methods handle those cases when applied to data.

@larrybradley
Copy link
Member Author

Another question: should we really have a test for 0.5?
It's a fractional float. Is it really guaranteed to round the way you have in the test on all machines?

I'm not actually sure.

@cdeil
Copy link
Member

cdeil commented Nov 28, 2016

I'm not actually sure.

+1 to remove that test and then merge.

@larrybradley
Copy link
Member Author

@cdeil Done!

@larrybradley larrybradley self-assigned this Nov 28, 2016
Copy link
Member

@cdeil cdeil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm merging this now.

@larrybradley - Should we leave the 0.5 rounding example in the docstring or also remove that?
We can always take it out in master

@cdeil cdeil merged commit dc40b72 into astropy:master Nov 28, 2016
@larrybradley larrybradley deleted the bounding_boxes branch November 28, 2016 19:14
@larrybradley
Copy link
Member Author

@cdeil I removed the 0.5 rounding example in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants