imagej / imagej-legacy

ImageJ+ImageJ2 compatibility layer
https://imagej.net/libs/imagej-legacy
BSD 2-Clause "Simplified" License
16 stars 25 forks source link

ResultsTableWrapper has no iterator implementation #189

Open imagejan opened 6 years ago

imagejan commented 6 years ago

While the following script works as expected:

import net.imagej.table.DefaultGenericTable

t = new DefaultGenericTable()

println t // []

it will fails when using a ResultsTableWrapper like this:

#@ ConvertService convert
#@ ResultsTable rt

import net.imagej.table.Table

tableWrapper = convert.convert(rt, Table.class)
println tableWrapper

While the returned object is indeed a ResultsTableWrapper, the stack trace indicates that there's no iterator() implementation:

java.lang.UnsupportedOperationException: iterator()
``` at net.imagej.legacy.convert.ResultsTableWrapper.iterator(ResultsTableWrapper.java:376) at org.codehaus.groovy.runtime.InvokerHelper.formatList(InvokerHelper.java:683) at org.codehaus.groovy.runtime.InvokerHelper.toListString(InvokerHelper.java:787) at org.codehaus.groovy.runtime.InvokerHelper.toListString(InvokerHelper.java:775) at org.codehaus.groovy.runtime.InvokerHelper.toListString(InvokerHelper.java:764) at org.codehaus.groovy.runtime.InvokerHelper.toString(InvokerHelper.java:129) at org.codehaus.groovy.runtime.DefaultGroovyMethods.println(DefaultGroovyMethods.java:654) at org.codehaus.groovy.runtime.dgm$502.doMethodInvoke(Unknown Source) at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1218) at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1027) at org.codehaus.groovy.runtime.InvokerHelper.invokePojoMethod(InvokerHelper.java:913) at org.codehaus.groovy.runtime.InvokerHelper.invokeMethod(InvokerHelper.java:904) at groovy.lang.Script.println(Script.java:161) at jdk.internal.reflect.GeneratedMethodAccessor17.invoke(Unknown Source) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:564) at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:93) at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:325) at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1218) at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1027) at groovy.lang.DelegatingMetaClass.invokeMethod(DelegatingMetaClass.java:151) at org.scijava.plugins.scripting.groovy.GroovyScriptEngine$2.invokeMethod(GroovyScriptEngine.java:283) at org.codehaus.groovy.runtime.callsite.PogoMetaClassSite.callCurrent(PogoMetaClassSite.java:69) at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:52) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:154) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:166) at Script53.run(Script53.groovy:7) at org.scijava.plugins.scripting.groovy.GroovyScriptEngine.eval(GroovyScriptEngine.java:303) at org.scijava.plugins.scripting.groovy.GroovyScriptEngine.eval(GroovyScriptEngine.java:122) at java.scripting/javax.script.AbstractScriptEngine.eval(AbstractScriptEngine.java:264) at org.scijava.script.ScriptModule.run(ScriptModule.java:160) at org.scijava.module.ModuleRunner.run(ModuleRunner.java:168) at org.scijava.module.ModuleRunner.call(ModuleRunner.java:127) at org.scijava.module.ModuleRunner.call(ModuleRunner.java:66) at org.scijava.thread.DefaultThreadService$3.call(DefaultThreadService.java:238) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) at java.base/java.lang.Thread.run(Thread.java:844) ```

I suppose the issue is with the Table interface having an iterator() declaration which leads to the one of AbstractList not being taken, but I don't understand multiple inheritance enough to be able to fully explain this. @awalter17, @ctrueden maybe you have a clue here?

awalter17 commented 6 years ago

@imagejan The iterator() method will always throw a java.lang.UnsupportedOperationException because I overrode the method in ResultsTableWrapper to always throw an exception (see here). It's not implemented in ResultsTableColumnWrapper either. There are a number of methods in the wrappers which throw UnsupportedOperationException (which I should have put in the javadoc, sorry!).

That being said, there's (to the best of my knowledge) no reason not to make a custom Iterator. It just would have been more work, and at the time I didn't need that method.

If you're just trying to view the tables, in the interim you could declare them as outputs and then they should display fine.

imagejan commented 6 years ago

@awalter17 the trigger for this issue was this forum post, actually. I wanted to set up an example of doing table calculations in an IJ2 table after retrieving it via script parameters from an IJ1 ResultsTable. But even the simplest operations on the wrapped table resulted in this iterator issue.

Is there a reason why we can't use the inherited List iterator and override the iterator() implementation here?

I was able to getColumnCount() and getRowCount() on the wrapped table, so I wonder why inheriting the iterator wouldn't work. I think it would be great to have the full Table functionality with wrapped IJ1 ResultsTables.

awalter17 commented 6 years ago

Is there a reason why we can't use the inherited List iterator and override the iterator() implementation here?

The ResultsTableWrapper doesn't extend any concrete implementation of List, so we can't use something like super.iterator(). If we wanted to implement the iterator() method we'd probably need to create our own Iterator implementation, or change ResultsTableWrapper to extend a concrete implementation of List (similar to AbstractTable).

Either way, it gets a bit complicated because the Tables iterator is typed on Column which doesn't exist in the IJ1 ResultsTable. So every time we want a Column we need to wrap the corresponding array in the ResultsTable.

I think it would be great to have the full Table functionality with wrapped IJ1 ResultsTables.

I agree, but it may not be possible to preserve all functionality due to differences between the IJ2 and IJ1 API.

That being said, there are methods which I didn't implement that could be implemented in the wrapper with a bit of work. At the time, I only implemented methods which had somewhat direct equivalents in the IJ1 ResultsTable. My use case, (I think it was displaying IJ1 tables in a jupyter notebook) was focused on displaying the tables not mutating them. So I believe most of the accessor methods work, but a lot of the mutators do not 😿

imagejan commented 6 years ago

Thanks for your explanations, @awalter17! That helps a lot.

I wonder whether it would be desirable to wrap all columns as ResultsTableColumnWrapper in the constructor and then be able to provide a functional List (e.g. by extending AbstractList as you suggest). I might look into it while migrating the table classes to scijava-table (see https://github.com/scijava/scijava-common/issues/314), but it might take a while.

So I believe most of the accessor methods work, but a lot of the mutators do not

That's funny, because appendColumn worked fine when I tried, but simply trying to iterate over the columns to display their names created the aforementioned problems.

imagejan commented 4 years ago

I just stumbled upon this issue again while trying if a quick solution for this forum question exists with SciJava tables.

I would have been great if you could just write:

#@ ResultsTable rt
#@ ConvertService cs
#@ OpService ops

import org.scijava.table.Table

table = cs.convert(rt, Table.class)

targetColumn = table.get(0)
columnHistogram = ops.image().histogram(targetColumn)
mode = ops.stats().max(columnHistogram)

But whenever you want to access any column of the table or similar, you get:

java.lang.UnsupportedOperationException: iterator()
    at net.imagej.legacy.convert.ResultsTableWrapper.iterator(ResultsTableWrapper.java:374)

or

java.lang.UnsupportedOperationException: iterator()
    at net.imagej.legacy.convert.ResultsTableColumnWrapper.iterator(ResultsTableColumnWrapper.java:97)