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

DefaultOverlaySerivce.getSelectionBounds miscalculates height and width #83

Open
gselzer opened this issue Jan 15, 2019 · 2 comments
Open

Comments

@gselzer
Copy link
Contributor

gselzer commented Jan 15, 2019

In this method the selection bounds return object is given as a new RealRect(xMin, yMin, xMax - xMin, yMax - yMin) (see constructor). xMin/xMax/yMin/yMax are found by calling realMin(dimension) or realMax(dimension) on the Data object of the input ImageDisplay's DataView or from the ImageDisplay's bounds itself. For example, an image image of size 256/256, xMin/yMin will be 0 and xMax/yMax will be 255. This causes image.width and image.height, according to the above, to be 255, which is wrong, since it causes plugins like this one to not calculate the last column/row (see the driving for-loop here).

@imagejan
Copy link
Member

For an image of 256x256 pixels, the real width and height are actually 255.0 (max-min), since pixels are just sample points, not squares, right? Maybe the API should be changed to return a net.imglib2.roi.geom.real.ClosedWritableBox here instead of a RealRect? The closed boundary would make sure that all border pixels are included in the selection.

@gselzer
Copy link
Contributor Author

gselzer commented Jan 16, 2019

@imagejan yeah, I was thinking about that when I wrote the issue. I suppose it depends on how we are defining width/height here, as number of pixels or as a measure of distance (I was naively assuming the former). I just took a look at some of the other plugins relying on this method and some (like this one) assume width/height to be as you suggested, not like Sharpen/Smooth.

I am also hesitant to change the API of this method since it would break all of the plugins that might be calling it. Now that I am doing some more digging, maybe this is more of an issue with Neighborhood3x3Operation, since other plugins that call this method but do not use Neighborhood3x3Operation (like the one linked above) do not look like they have this issue. And, seeing as how some of these plugins using Neighborhood3x3Operation are being converted into Ops anyways (see imagej/imagej-ops#557 and imagej/imagej-ops#556), maybe we should just deprecate these faulty plugins, which would also keep reproducability

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

No branches or pull requests

2 participants