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

Fixed getLine() in ImageProcessor.java, added test plugin (plugins/wi… #34

Closed
wants to merge 1 commit into from
Closed

Conversation

imagingbook
Copy link

Hello Wayne,
I made a minor modification to the getLineMethod() in ImageProcessor to fix the problem you mentioned (#30). Things should work now as expected, in particular, the test case

ip = new ByteProcessor(100,100);
ip.setInterpolate(true);
values = ip.getLine(100.0,100.0,100.0,101.1);
print("values.length: " + values.length);

now returns a sequence of proper length 2. I also added a Java plugin for testing this behavior.

Thanks,
Wilhelm

@ctrueden
Copy link
Member

ctrueden commented May 3, 2017

I also added a Java plugin for testing this behavior.

Note that there are a large number of unit tests for ImageJ 1.x in the imagej/ij1-tests repository. Perhaps this test could be added there as a unit test.

@rasband
Copy link
Member

rasband commented May 4, 2017

Thanks for fixing this problem. However, I will probably continue to use the existing version of getLine() because there my be other compatibility problems and because, for non-interpolated lines, the new version is three times slower.

@rasband rasband closed this May 4, 2017
@imagingbook
Copy link
Author

@rasband: The additional execution time (for non-interpolated lines) is probably caused by autoboxing the float values for insertion into the list. One could try to run the iterator twice, once to determine the number of line pixels and once to fill the final double[] array directly, thus avoiding boxing/unboxing. How much absolute time are we talking about, does it really matter?

I understand the problem that modified code may behave differently, but (as noted in #30) the current getLine() method is algorithmically flawed and should be fixed in my opinion. It is Wayne's decision whether to change it or to live with the wrong (though accepted?) behavior -- I tend toward the first. Still hope I have not been wasting my time ...

@ctrueden: Thanks for pointing this out, I was not aware of this test repo. I suppose the existing tests for line drawing should be revised (they probably still test for the old behavior).

Related to this: What is the recommended IDE setup for building/debugging ImageJ1? I am used to Eclipse, but the IJ1 repo contains an nbproject folder, so I tried NetBeans, but apparently the setup (for NetBeans) is outdated.

@tferr
Copy link

tferr commented May 4, 2017

I came across the wrong systematic offset issue when converting SNT traces to line ROIs. I too think it would be important to have this addressed. I would favor to have @imagingbook fix implemented, but if backwards compatibility is really an issue, can we at least get to a compromise? E.g., by renaming the new methods to getLine2() , getLinePrecise(), getLineWilhelm() (or equivalent) without overriding the old ones?
I know it just adds clutter, but at least such methods would be accessible.

@imagingbook
Copy link
Author

imagingbook commented May 4, 2017

Great, I love getLineWilhelm()... Overloading the method (e.g., with an additional boolean parameter) might be another option.

@rasband
Copy link
Member

rasband commented May 4, 2017

@tferr, do you have a small script, or plugin, that reproduces the wrong offset issue with the getLine() method?

@rasband
Copy link
Member

rasband commented May 4, 2017

@tferr, I can reproduce the wrong offset problem with non-interpolated lines, so no need for a test script or plugin.

@rasband
Copy link
Member

rasband commented May 5, 2017

In the latest ImageJ daily build (1.51o1), I modified getLine() to use the offset fix that's in the Line PointIterator. I did not include the PointIterator's duplicate point fix because that would cause the profile plotter to fail. The profile plotter assumes the length of the line is equal to the length of the array (minus one) returned by getLine().

@imagingbook
Copy link
Author

Thanks. Alternatively, one could modify Line.PointIterator to optionally turn off skipping duplicates.

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

Successfully merging this pull request may close these issues.

4 participants