imagej / imagej-scripting

ImageJ-specific applications of the SciJava script languages
Other
25 stars 15 forks source link

Add Manual_Registration.py script #26

Closed hadim closed 7 years ago

hadim commented 7 years ago

ping @bnorthan

hadim commented 7 years ago

Thank you for the review.

I forgot that ImageJ should not be used (see #18). I'll remove it.

For the test, we can definitively add a test dependency with IJ1 stuff but the thing is how to pass the RoiManager instance to the script ? All parameters need to be added to Map<String, Object> parameters = new HashMap<>();. Can I just add RoiManager to it ? I'll try that but since it's not an IJ2 object I don't know if it can work.

I'll push modifications later in the week.

hadim commented 7 years ago

I have fixed most of your concerns @stelfrich.

Obviously # @RoiManager rm annotation does not work. If you have ideas how to use RoiManager inside a test, feel free to tell me.

ctrueden commented 7 years ago

Obviously # @RoiManager rm annotation does not work. If you have ideas how to use RoiManager inside a test, feel free to tell me.

Is there some reason RoiManager.getInstance() does not do the job? That is the IJ1 way: a static singleton.

hadim commented 7 years ago

I didn't know that RoiManager called outside of scriptService.run() could be reused inside the script. The script now correctly detects the ROIs added inside the test.

But two things :


# @DatasetIOService datasetIOService
# @ ImageJ ij

from ij.plugin.frame import RoiManager
from ij.gui import PointRoi
from ij.gui import Roi

testPath = "8bit-signed&pixelType=int8&axes=X,Y,TIME&lengths=10,10,10.fake"
data = datasetIOService.open(testPath)

rm = RoiManager.getRoiManager()
point1 = PointRoi(2, 2)
point1.setPosition(1)
rm.addRoi(point1)
point2 = PointRoi(5, 9)
point2.setPosition(3)
rm.addRoi(point2)
point3 = PointRoi(7, 1)
point3.setPosition(7)
rm.addRoi(point3)

ij.ui().show(data)

It works perfectly so I don't understand why the script fails. I have added the ij dep in the pom.xml.

ctrueden commented 7 years ago
# @ ImageJ ij

Should be:

# @ImageJ ij

Although: is there some reason you don't instead do:

# @UIService ui
...
ui.show(...)

? Then you do not need to depend on net.imagej:imagej toplevel.

hadim commented 7 years ago

This script is only here to reproduce the failing test I have in the Java test file.

It need te be run before the real script "Manual_Registration.py"

stelfrich commented 7 years ago

Or do you want the test to be UI independent?

If not, the test won't pass on Travis, I fear:

java.awt.HeadlessException: 
No X11 DISPLAY variable was set, but this program performed an operation which requires it.
    at net.imagej.scripting.ImageJ2ScriptTest.testManualRegistrationScript(ImageJ2ScriptTest.java:229)

This script is only here to reproduce the failing test I have in the Java test file.

The test also runs perfectly fine from within Eclipse. Here's the exception when running mvn test:

Traceback (most recent call last):
  File "/home/stefan/workspace_imagej/imagej-scripting/target/classes/script_templates/ImageJ2/Manual_Registration.py", line 67, in <module>
    translated_frame = ops.transform().translate(padded_frame, [dx, dy, 0])
    at net.imglib2.transform.integer.MixedTransform.setInverseTranslation(MixedTransform.java:155)
    at net.imglib2.view.Views.translate(Views.java:415)
    at net.imagej.ops.transform.translateView.DefaultTranslateView.calculate(DefaultTranslateView.java:56)
    at net.imagej.ops.transform.translateView.DefaultTranslateView.calculate(DefaultTranslateView.java:47)
    at net.imagej.ops.special.function.UnaryFunctionOp.run(UnaryFunctionOp.java:75)
    at net.imagej.ops.special.function.AbstractUnaryFunctionOp.run(AbstractUnaryFunctionOp.java:59)
    at org.scijava.command.CommandModule.run(CommandModule.java:205)
    at net.imagej.ops.OpEnvironment.run(OpEnvironment.java:940)
    at net.imagej.ops.OpEnvironment.run(OpEnvironment.java:156)
    at net.imagej.ops.transform.TransformNamespace.translate(TransformNamespace.java:792)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)

java.lang.AssertionError: java.lang.AssertionError
stelfrich commented 7 years ago

@hadim I went ahead and fixed the issue with dimension (plus some minor changes, e.g., the namespace methods I have talked about).

stelfrich commented 7 years ago

For completeness sake: I have tested creating a RoiManager via RoiManager(false) and injecting that as the singleton instance via the Reflection API without success.

hadim commented 7 years ago

Same here : Tests with Netbeans fails while mvn test works well.

For Travis I have added

before_script:
  - "export DISPLAY=:99.0"
  - "sh -e /etc/init.d/xvfb start"
  - sleep 3 # give xvfb some time to start

to try to make it work.

hadim commented 7 years ago

I think the Travis trick worked.

So we only have the Netbeans/Eclipse bug remaining.

stelfrich commented 7 years ago

So we only have the Netbeans/Eclipse bug remaining.

Tests are passing in Eclipse for me.

ctrueden commented 7 years ago

I would prefer to avoid Xvfb, unless you feel it is absolutely necessary for proper testing.

This thread is long, and I got a little confused—what is the problem exactly with the tests running in headless mode? What exactly cannot be tested properly? Can someone post a minimal example and explain the desired behavior, and how that differs from reality? Or point to the comment link above which does so?

@hadim As an aside though, this is a neat trick, so I added it to the Travis page. :grin:

stelfrich commented 7 years ago

@ctrueden When ImageJ2ScriptTest.testManualRegistrationScript is executed on Travis, the following exception is raised (also see the Travis build log) when not changing the configuration to enable Xvfb:

java.awt.HeadlessException: 
No X11 DISPLAY variable was set, but this program performed an operation which requires it.
    at net.imagej.scripting.ImageJ2ScriptTest.testManualRegistrationScript(ImageJ2ScriptTest.java:229)

As far as I can tell this is due to the RoiManager implementation as a java.awt.Frame and all its AWT references? The exception is also thrown when using new RoiManager(false) (which creates a new instance but doesn't show it).

ctrueden commented 7 years ago

@stelfrich OK, so you guys are trying to add some IJ1-specific stuff, particularly the RoiManager which is known to not work headless as you know from that thread and others.

Firstly, can we please split the IJ1-specific stuff and tests to their own .java file(s)? It bugs me that the unit test class is called ImageJ2ScriptTest but it now has ImageJ-1.x-specific things in it. Why not have a separate ImageJ1ScriptTest or similar?

Secondly, how to fix the RoiManager headlessness problem:

  1. We could add support for PlugInFrame in ImageJ Legacy, so that it no longer extends Frame but instead gets replaced, analogous to how we fix GenericDialog [1, 2]. Then RoiManager would not extend Frame transitively anymore. This would be the "best" solution but also the most work.

  2. Alternately, can the Manual_Registration.py be changed to use overlays instead of the ROI manager? That is the typical advice given in these circumstances.

hadim commented 7 years ago

The idea behind the scripts in the Imagej2 folder is to provide scripts using only IJ2 features. The only remaining IJ1 stuff I still use (being in scripts or in plugins) is the IJ1 RoiManager.

The RoiManager is super useful in Fiji to be able to manage and draw a set of ROIs that can be later used in scripts/plugins.

I really would like to find an IJ2 equivalence to it but I don't think it exists yet. I guess we need to wait that imglib2-roi is stable enough.

Also, what is the idea with the OverlayManager ? Is it supposed to be used with imglib2-roi ?

I would be ok to use it in all my scripts/plugins but the OverlayManager window does not work in Fiji. I got the following error when trying to open it :

The module "net.imagej/ui.swing.commands.OverlayManager" is invalid:
-invalid duplicate parameter: private org.scijava.Context net.imagej.ui.swing.commands.OverlayManager.context

So I propose to just comment the test ImageJ2ScriptTest.testManualRegistrationScript for now and merge this PR.

hadim commented 7 years ago

Also when I add overlays using Ctrl+B in Fiji, the following script only returns a list with null overlays :

# @OverlayService rm
# @ImageDisplay disp

print(disp)
print(rm.getOverlays(disp))
plugin:class net.imagej.display.DefaultImageDisplay: type=interface net.imagej.display.DataView, name=FakeTracks.tif, objects={net.imagej.display.DefaultDatasetView@1c1e0948, net.imagej.display.DefaultOverlayView@218e5ef7, net.imagej.display.DefaultOverlayView@25b1ce1e, net.imagej.display.DefaultOverlayView@60b354cc, net.imagej.display.DefaultOverlayView@2b3ff61, net.imagej.display.DefaultOverlayView@51197026, net.imagej.display.DefaultOverlayView@7af38525}
[null, null, null, null, null, null]

As you can see some DefaultOverlayView exist.

ctrueden commented 7 years ago

The RoiManager is super useful in Fiji to be able to manage and draw a set of ROIs that can be later used in scripts/plugins.

I really would like to find an IJ2 equivalence to it but I don't think it exists yet. I guess we need to wait that imglib2-roi is stable enough.

Indeed, there is no equivalent yet. The Overlay Manager you mention is years old—written against the "old" ImageJ2 data model of Overlay which extends Data. That subsystem is slated for redesign as part of the "rich image" rework. I would advise against using net.imagej.overlay.Overlay and friends, since the IJ1 stuff works (mostly) just fine until the new data model is in place.

I would be ok to use it in all my scripts/plugins but the OverlayManager window does not work in Fiji.

I am sorry if I was unclear earlier: when I said "use overlays" I meant ImageJ 1.x overlays, not the ImageJ2 ones. I know it is horribly confusing, and again, I apologize.

hadim commented 7 years ago

Ok, I understand better now.

For now, I will just merge this PR without any IJ1 tests and wait for the ROI support in IJ2 to gets better.

What would be the future of RoiManager-like feature support in Fiji ? For what I have understood the swing UI is not a priority anymore (and probably will never be) so IJ1 UI will probably be used for years.

The RoiManager widget is nice and has all the features needed for users to interact with ROIs. So would it be possible to develop a "bridge" between the IJ1 RoiManager and the soon-to-be-coming IJ2 ROIs API ? So people will still be able to interact with the IJ1 RoiManager and developers will have the choice to use the IJ1 API or the IJ2 RoiManager bridge to retrieve ROIs in different format (IJ1 or IJ2).

ctrueden commented 7 years ago

What would be the future of RoiManager-like feature support in Fiji ? For what I have understood the swing UI is not a priority anymore (and probably will never be) so IJ1 UI will probably be used for years.

It does not need to be UI-specific, actually. The OverlayService or ROIService or whatever we end up calling it can give you API for querying the ROIs currently available to the context, and/or to a specific image. Unlike ImageJ 1.x, the imageJ2 rich image data model will emphasize attaching "trees" of ROIs to specific images rather than globally, so you can simply query an image directly to get its ROIs (e.g. results from something like Analyze Particles, or CCA). And transferring ROIs between images would be done by attaching some ROI tree to a new rich image. We could still have a global "tell me about all ROIs attached to all images" style thing, but I'm not sure it would be necessary anymore.

As for a UI that actually lets you browse these ROIs easily: this can be part of imagej-ui-swing in the medium term, available as a new-and-improved ROI browser in the ImageJ Legacy UI—which already includes some Swing things plucked from the "new" ImageJ Swing UI.

The RoiManager widget is nice and has all the features needed for users to interact with ROIs. So would it be possible to develop a "bridge" between the IJ1 RoiManager and the soon-to-be-coming IJ2 ROIs API ?

Yes, we should either: A) make the ROIs in the RoiManager automagically translate back and forth between ImageJ2, similar to how ImagePlus and ImageDisplay do now for images themselves; or B) make a distinct "ROI Browser" or similar, which has all the same features, but works with ROIs on an image-specific level. Happy to discuss further later once the ImageJ2 rich image + ROI data model is closer to fruition.