Simplify command classes and examples #106

emilmelnikov · 2023-07-03T18:39:10Z

Merge commands and executors for less indirection
Move subprocess running, temporary file management and common parameters to the CommandBase class
Simplify and merge together demo classes
Add a simple pixel classification ImageJ macro

Partially supersedes #84 without adding the features table feature.

emilmelnikov · 2023-07-03T18:51:11Z

This PR misses the Tracking demo because data files used in the old demo are not included in the project resources. This demo, along with other workflows, will be added later in another PR.

* Merge commands and executors for less indirection * Move subprocess running, temporary file management and common parameters to the CommandBase class * Simplify and merge together demo classes * Add a simple pixel classification ImageJ macro

k-dominik

Hey @emilmelnikov,

sorry for taking some time to look at this. It looks really nice, I appreciate the cleanup and simplification, as, e.g. with handling of temporary files and the more elegant way of extending/creating new commands!

I still need to do some testing, like, if some old macros that I have still work :)

It would be nice to add a paragraph about running the plugin from java (as in demo.java) to the readme.

For future development:

in order to have a little more uniform flow:
- add output types to workflows other than Pixel Classification, e.g. Probabilities, Predictions, ObjectIDs to object classification
- add stage 1 outputs to Autocontext command

src/main/java/org/ilastik/ilastik4ij/cmd/CommandBase.java

src/main/java/org/ilastik/ilastik4ij/cmd/Options.java

src/main/java/org/ilastik/ilastik4ij/cmd/CommandBase.java

k-dominik · 2023-07-11T13:14:03Z

Okay I ran a few macros and they all still seem to work as before 👍

emilmelnikov · 2023-07-11T13:36:09Z

Okay I ran a few macros and they all still seem to work as before 👍

Do you know how to read a "non-standard" (e.g. probabilities) image in a macro? For me it did not work because it needed to be imported via BioFormats that shows a pop-up dialog which doesn't work properly in the macro environment.

emilmelnikov · 2023-07-12T09:27:27Z

Do you know how to read a "non-standard" (e.g. probabilities) image in a macro? For me it did not work because it needed to be imported via BioFormats that shows a pop-up dialog which doesn't work properly in the macro environment.

After discussing this issue with @k-dominik, it turns out that some of our test TIFF files in this repo are not ImageJ-compatible. Re-exporting them with ImageJ should solve the problem.

Co-authored-by: Dominik Kutra <k-dominik@users.noreply.github.com>

OptionsPlugin uses Java Preferences system for persisting options. This system uses fully qualified class names as keys for data persistence. Co-authored-by: Dominik Kutra <k-dominik@users.noreply.github.com>

* Show progress bars and spinners in StatusService during potentially lengthy operations * Instead of using LogService directly, log to a class-name-scoped logger

emilmelnikov · 2023-07-14T16:18:52Z

Now everything that ilastik prints (both stdout and stderr) goes into the INFO log level. This prevents a log window from popping up when ilastik prints warnings to stderr, which is very common (it occurs if ilastik cannot find any solvers in the system).

We have other non-workflow commands that should not derive from CommandBase.

Also add more docstrings and comments.

GitHub now shows headings in Markdown files, so it's not necessary to create table of contents manually

README.md

btbest

I've had a browse through the code changes, and this looks like a really good refactor. I haven't actually gone through the setup to try running this now, so let me know if I should as that will probably take me a couple more days...

One leftover question from me regarding the compatibility note in the readme:

Compatibility note:: We try to keep the plugin backwards compatible. Some versions of ilastik are not compatible with the current plugin: 1.3.3b2, 1.3.3rc2, 1.3.3, 1.3.3post1, 1.3.3post2. We recommend to update to the latest stable version!. If you need to stick with 1.3.3x for some reason, please use 1.3.3post3.

Is that still accurate now; are we able to make statements about compatibility or lack thereof after these changes? As far as I can follow, we are calling ilastik pretty much in the same way as before, so I guess most likely the compatibility hasn't changed. But I also don't really know why the plugin is incompatible with certain 1.3.3 subversions anyway :)

k-dominik

the spinner is pretty cool :) Especially that it updates the status bar, so even on mouse moves one will be reminded of the fact that ilastik is running!

Let's bump version to at least 1.9.0 when we release this.

k-dominik · 2023-07-21T14:08:25Z

src/main/java/org/ilastik/ilastik4ij/hdf5/Hdf5DataSetWriter.java

@@ -205,11 +205,11 @@ private void createMDArray(IHDF5Writer writer, long[] datasetDims, int[] blockSi
    }

    private void writeARGB(IHDF5Writer writer, long[] datasetDims) {


not your code, I know, just curious what one would ever do with an argb image, and also how or why we would treat it differently than any other multi channel image 🤷 .

I'm not completely sure, but a cursory look at writeARGB tells me that this is because ARGBType is a possible element type on an image which encodes 4 channels in an element itself, not in the shape of an image, hence the special handling.

k-dominik · 2023-07-21T14:52:19Z

src/main/java/org/ilastik/ilastik4ij/ui/IlastikOptions.java


    @Parameter(label = "Number of Threads ilastik is allowed to use.\nNegative numbers means no restriction")
-    private int numThreads = -1;
+    public int numThreads = -1;


More a question: previously, options were accessed with getters and setters to private variables, now everything is public. So... OptionsPlugin seems to support both styles?

Values of all fields annotated with @Parameter are somehow populated by the system.

Previously getters just called load() before returning field values, but didn't find the reason to do this because load() and save() are a part of options API, and I think it's better to just read/write them directly. We can't change their names anyway.

k-dominik · 2023-07-21T15:01:03Z

src/test/java/org/ilastik/ilastik4ij/Demo.java

+
+import net.imagej.Dataset;
+import net.imagej.ImageJ;
+import org.ilastik.ilastik4ij.workflow.ObjectClassificationCommand;


I'm not sure if anyone used this from java before, but we should at least mention in some release notes, that the structure has changed a bit and point to the demo file that you have provided so nicely before releasing :)

k-dominik · 2023-07-21T15:36:42Z

src/main/java/org/ilastik/ilastik4ij/workflow/TrackingCommand.java

+            return Collections.singletonMap("prediction_maps", inputProbOrSegImage);
+        }
+        if (ROLE_SEGMENTATION.equals(secondInputType)) {
+            return Collections.singletonMap("binary_image", inputProbOrSegImage);


it might be more useful to have segmentation_image here already - I can't imagine that more people are using the tracking with learning workflow, than the "normal" one (or animal tracking).

xref: ilastik/ilastik#2730

k-dominik · 2023-07-21T15:43:02Z

src/main/java/org/ilastik/ilastik4ij/workflow/WorkflowCommand.java

+
+    @Override
+    public final void run() {
+        logger = logService.subLogger(getClass().getCanonicalName(), LogLevel.INFO);


what is the reason to not use the logger declared above, but now use something from logService? What is the difference using one vs. the other?

LogService#log logs into the root logger, Logger#log logs into some defined namespace (it our case it's fully-qualified class name). Unfortunately, log prefixes are not visible in the Info tab, but the Log tab (or some other log destination) shows them.

This reverts commit b96ae0d.

emilmelnikov requested review from wolny and k-dominik July 3, 2023 18:39

emilmelnikov force-pushed the simplify-commands branch from 1c9284f to 91247fe Compare July 3, 2023 18:45

emilmelnikov force-pushed the simplify-commands branch 4 times, most recently from 983117d to c58444d Compare July 3, 2023 19:30

Simplify command classes and examples

94506df

* Merge commands and executors for less indirection * Move subprocess running, temporary file management and common parameters to the CommandBase class * Simplify and merge together demo classes * Add a simple pixel classification ImageJ macro

emilmelnikov force-pushed the simplify-commands branch from c58444d to 94506df Compare July 3, 2023 19:33

k-dominik reviewed Jul 11, 2023

View reviewed changes

src/main/java/org/ilastik/ilastik4ij/cmd/CommandBase.java Outdated Show resolved Hide resolved

src/main/java/org/ilastik/ilastik4ij/cmd/Options.java Outdated Show resolved Hide resolved

src/main/java/org/ilastik/ilastik4ij/cmd/Options.java Outdated Show resolved Hide resolved

k-dominik reviewed Jul 11, 2023

View reviewed changes

src/main/java/org/ilastik/ilastik4ij/cmd/CommandBase.java Outdated Show resolved Hide resolved

Remove redundant command-line parameter

0c6e69c

Co-authored-by: Dominik Kutra <k-dominik@users.noreply.github.com>

emilmelnikov mentioned this pull request Jul 13, 2023

Show directory selection dialog on macOS #108

Closed

emilmelnikov and others added 2 commits July 13, 2023 14:30

Restore old options class name and command package

f177b09

OptionsPlugin uses Java Preferences system for persisting options. This system uses fully qualified class names as keys for data persistence. Co-authored-by: Dominik Kutra <k-dominik@users.noreply.github.com>

Improve user interaction during command execution

3ad7118

* Show progress bars and spinners in StatusService during potentially lengthy operations * Instead of using LogService directly, log to a class-name-scoped logger

emilmelnikov requested a review from k-dominik July 14, 2023 16:20

emilmelnikov added 2 commits July 15, 2023 09:25

Rename and move workflow commands

bf8ac6a

We have other non-workflow commands that should not derive from CommandBase.

Split WorkflowCommand#run into several steps

780c506

Also add more docstrings and comments.

emilmelnikov force-pushed the simplify-commands branch from 928baa0 to 780c506 Compare July 15, 2023 09:05

emilmelnikov added 4 commits July 15, 2023 11:08

Fix class name in the Demo options example

a05561a

Remove TOC from README

b96ae0d

GitHub now shows headings in Markdown files, so it's not necessary to create table of contents manually

Mention Demo in README

15d28d6

Make SPINNER_CHARS a string

11c1098

emilmelnikov requested a review from btbest July 19, 2023 13:48

k-dominik reviewed Jul 21, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

btbest reviewed Jul 21, 2023

View reviewed changes

k-dominik approved these changes Jul 21, 2023

View reviewed changes

Revert "Remove TOC from README"

8acfcd8

This reverts commit b96ae0d.

emilmelnikov merged commit 4aa8e3d into master Jul 27, 2023
1 check passed

emilmelnikov deleted the simplify-commands branch July 27, 2023 12:29

emilmelnikov mentioned this pull request Sep 22, 2023

Read object features table #84

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify command classes and examples #106

Simplify command classes and examples #106

emilmelnikov commented Jul 3, 2023

emilmelnikov commented Jul 3, 2023

k-dominik left a comment •

edited

Loading

k-dominik commented Jul 11, 2023

emilmelnikov commented Jul 11, 2023

emilmelnikov commented Jul 12, 2023 •

edited

Loading

emilmelnikov commented Jul 14, 2023

btbest left a comment

k-dominik left a comment

k-dominik Jul 21, 2023

emilmelnikov Jul 22, 2023

k-dominik Jul 21, 2023

emilmelnikov Jul 22, 2023

k-dominik Jul 21, 2023

k-dominik Jul 21, 2023

k-dominik Jul 21, 2023

emilmelnikov Jul 22, 2023

		@@ -205,11 +205,11 @@ private void createMDArray(IHDF5Writer writer, long[] datasetDims, int[] blockSi
		}

		private void writeARGB(IHDF5Writer writer, long[] datasetDims) {

Simplify command classes and examples #106

Simplify command classes and examples #106

Conversation

emilmelnikov commented Jul 3, 2023

emilmelnikov commented Jul 3, 2023

k-dominik left a comment • edited Loading

Choose a reason for hiding this comment

k-dominik commented Jul 11, 2023

emilmelnikov commented Jul 11, 2023

emilmelnikov commented Jul 12, 2023 • edited Loading

emilmelnikov commented Jul 14, 2023

btbest left a comment

Choose a reason for hiding this comment

k-dominik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-dominik left a comment •

edited

Loading

emilmelnikov commented Jul 12, 2023 •

edited

Loading