From ac6257370e9910aab31d084e214306390906371b Mon Sep 17 00:00:00 2001 From: Alan O'Callaghan Date: Tue, 6 Aug 2024 13:55:07 +0100 Subject: [PATCH] Leos comments (#44) * Address comments in #42 --- .../ext/instanseg/core/DetectionMeasurer.java | 1 - .../qupath/ext/instanseg/core/InstanSeg.java | 1 - .../ext/instanseg/core/InstanSegModel.java | 131 ++++++++++-------- .../ext/instanseg/core/MatTranslator.java | 17 ++- .../{ui => core}/PytorchManager.java | 34 ++--- .../core/TilePredictionProcessor.java | 16 ++- .../ext/instanseg/ui/InstanSegController.java | 86 +++++++----- .../ext/instanseg/ui/InstanSegExtension.java | 6 +- .../instanseg/ui/InstanSegPreferences.java | 11 +- .../ext/instanseg/ui/MessageTextHelper.java | 6 +- .../java/qupath/ext/instanseg/ui/Watcher.java | 17 ++- .../ext/instanseg/ui/strings.properties | 1 + 12 files changed, 181 insertions(+), 146 deletions(-) rename src/main/java/qupath/ext/instanseg/{ui => core}/PytorchManager.java (94%) diff --git a/src/main/java/qupath/ext/instanseg/core/DetectionMeasurer.java b/src/main/java/qupath/ext/instanseg/core/DetectionMeasurer.java index c66fc77..9a5585a 100644 --- a/src/main/java/qupath/ext/instanseg/core/DetectionMeasurer.java +++ b/src/main/java/qupath/ext/instanseg/core/DetectionMeasurer.java @@ -22,7 +22,6 @@ public class DetectionMeasurer { private final Collection shapeFeatures; private final double pixelSize; - private DetectionMeasurer(Collection compartments, Collection measurements, Collection shapeFeatures, diff --git a/src/main/java/qupath/ext/instanseg/core/InstanSeg.java b/src/main/java/qupath/ext/instanseg/core/InstanSeg.java index a8bdb23..0947d6a 100644 --- a/src/main/java/qupath/ext/instanseg/core/InstanSeg.java +++ b/src/main/java/qupath/ext/instanseg/core/InstanSeg.java @@ -165,7 +165,6 @@ public Builder currentImageData() { return this; } - /** * Set the channels to be used in inference * @param channels A collection of channels to be used in inference diff --git a/src/main/java/qupath/ext/instanseg/core/InstanSegModel.java b/src/main/java/qupath/ext/instanseg/core/InstanSegModel.java index 613f67a..c01dbe4 100644 --- a/src/main/java/qupath/ext/instanseg/core/InstanSegModel.java +++ b/src/main/java/qupath/ext/instanseg/core/InstanSegModel.java @@ -49,7 +49,6 @@ private InstanSegModel(BioimageIoSpec.BioimageIoModel bioimageIoModel) { this.name = model.getName(); } - private InstanSegModel(URL modelURL, String name) { this.modelURL = modelURL; this.name = name; @@ -75,26 +74,80 @@ public static InstanSegModel fromName(String name) { throw new UnsupportedOperationException("Fetching models by name is not yet implemented!"); } - public BioimageIoSpec.BioimageIoModel getModel() { - if (model == null) { - try { - fetchModel(); - } catch (IOException e) { - // todo: exception handling here, or...? - throw new RuntimeException(e); - } - } - return model; - } - + /** + * Get the pixel size in the X dimension. + * @return the pixel size in the X dimension. + */ public Double getPixelSizeX() { return getPixelSize().get("x"); } + /** + * Get the pixel size in the Y dimension. + * @return the pixel size in the Y dimension. + */ public Double getPixelSizeY() { return getPixelSize().get("y"); } + /** + * Get the path where the model is stored on disk. + * @return A path on disk, or an exception if it can't be found. + */ + public Path getPath() { + if (path == null) { + fetchModel(); + } + return path; + } + + @Override + public String toString() { + return getName(); + } + + /** + * Check if a path is (likely) a valid InstanSeg model. + * @param path The path to a folder. + * @return True if the folder contains an instanseg.pt file and an accompanying rdf.yaml. + * Does not currently validate the contents of either, but may in future check + * the yaml contents and the checksum of the pt file. + */ + public static boolean isValidModel(Path path) { + // return path.toString().endsWith(".pt"); // if just looking at pt files + if (Files.isDirectory(path)) { + return Files.exists(path.resolve("instanseg.pt")) && Files.exists(path.resolve("rdf.yaml")); + } + return false; + } + + /** + * The number of tiles that failed during processing. + * @return The count of the number of failed tiles. + */ + public int nFailed() { + return nFailed; + } + + /** + * Get the model name + * @return A string + */ + String getName() { + return name; + } + + /** + * Retrieve the BioImage model spec. + * @return The BioImageIO model spec for this InstanSeg model. + */ + BioimageIoSpec.BioimageIoModel getModel() { + if (model == null) { + fetchModel(); + } + return model; + } + private Map getPixelSize() { // todo: this code is horrendous var map = new HashMap(); @@ -105,45 +158,23 @@ private Map getPixelSize() { return map; } - private void fetchModel() throws IOException { + private void fetchModel() { if (modelURL == null) { throw new NullPointerException("Model URL should not be null for a local model!"); } downloadAndUnzip(modelURL, getUserDir().resolve("instanseg")); } - private static void downloadAndUnzip(URL url, Path localDirectory) throws IOException { + private static void downloadAndUnzip(URL url, Path localDirectory) { // todo: implement } - private static Path getUserDir() { Path userPath = UserDirectoryManager.getInstance().getUserPath(); Path cachePath = Paths.get(System.getProperty("user.dir"), ".cache", "QuPath"); return userPath == null || userPath.toString().isEmpty() ? cachePath : userPath; } - public String getName() { - return name; - } - - public Path getPath() { - if (path == null) { - try { - fetchModel(); - } catch (IOException e) { - // todo: handle here, or...? - throw new RuntimeException(e); - } - } - return path; - } - - @Override - public String toString() { - return getName(); - } - void runInstanSeg( ImageData imageData, Collection pathObjects, @@ -158,7 +189,8 @@ void runInstanSeg( TaskRunner taskRunner) { nFailed = 0; - Path modelPath = getPath().resolve("instanseg.pt"); + Path modelPath; + modelPath = getPath().resolve("instanseg.pt"); int nPredictors = 1; // todo: change me? @@ -227,27 +259,4 @@ private static void printResourceCount(String title, BaseNDManager manager) { manager.debugDump(2); } - /** - * Check if a path is (likely) a valid InstanSeg model. - * @param path The path to a folder. - * @return True if the folder contains an instanseg.pt file and an accompanying rdf.yaml. - * Does not currently validate the contents of either, but may in future check - * the yaml contents and the checksum of the pt file. - */ - public static boolean isValidModel(Path path) { - // return path.toString().endsWith(".pt"); // if just looking at pt files - if (Files.isDirectory(path)) { - return Files.exists(path.resolve("instanseg.pt")) && Files.exists(path.resolve("rdf.yaml")); - } - return false; - } - - /** - * The number of tiles that failed during processing. - * @return The count of the number of failed tiles. - */ - public int nFailed() { - return nFailed; - } - } diff --git a/src/main/java/qupath/ext/instanseg/core/MatTranslator.java b/src/main/java/qupath/ext/instanseg/core/MatTranslator.java index 250d07c..efe0038 100644 --- a/src/main/java/qupath/ext/instanseg/core/MatTranslator.java +++ b/src/main/java/qupath/ext/instanseg/core/MatTranslator.java @@ -13,12 +13,18 @@ class MatTranslator implements Translator { private final String inputLayoutNd; private final String outputLayoutNd; - private final boolean nucleiOnly; + private final boolean firstChannelOnly; - public MatTranslator(String inputLayoutNd, String outputLayoutNd, boolean nucleiOnly) { + /** + * Create a translator from InstanSeg input to output. + * @param inputLayoutNd N-dimensional output specification + * @param outputLayoutNd N-dimensional output specification + * @param firstChannelOnly Should the model only be concerned with the first output channel? + */ + MatTranslator(String inputLayoutNd, String outputLayoutNd, boolean firstChannelOnly) { this.inputLayoutNd = inputLayoutNd; this.outputLayoutNd = outputLayoutNd; - this.nucleiOnly = nucleiOnly; + this.firstChannelOnly = firstChannelOnly; } /** @@ -31,9 +37,8 @@ public NDList processInput(TranslatorContext ctx, Mat input) { var manager = ctx.getNDManager(); var ndarray = DjlTools.matToNDArray(manager, input, inputLayoutNd); var out = new NDList(ndarray); - if (nucleiOnly) { - var inds = new int[]{1, 1}; - inds[1] = 0; + if (firstChannelOnly) { + var inds = new int[]{1, 0}; var array = manager.create(inds, new Shape(2)); var arrayCPU = array.toDevice(Device.cpu(), false); out.add(arrayCPU); diff --git a/src/main/java/qupath/ext/instanseg/ui/PytorchManager.java b/src/main/java/qupath/ext/instanseg/core/PytorchManager.java similarity index 94% rename from src/main/java/qupath/ext/instanseg/ui/PytorchManager.java rename to src/main/java/qupath/ext/instanseg/core/PytorchManager.java index 5407367..be64c11 100644 --- a/src/main/java/qupath/ext/instanseg/ui/PytorchManager.java +++ b/src/main/java/qupath/ext/instanseg/core/PytorchManager.java @@ -1,4 +1,4 @@ -package qupath.ext.instanseg.ui; +package qupath.ext.instanseg.core; import ai.djl.engine.Engine; import ai.djl.engine.EngineException; @@ -15,15 +15,28 @@ /** * Helper class to manage access to PyTorch via Deep Java Library. */ -class PytorchManager { +public class PytorchManager { private static final Logger logger = LoggerFactory.getLogger(PytorchManager.class); + /** + * Get the PyTorch engine, downloading if necessary. + * @return the engine if available, or null if this failed + */ + public static Engine getEngineOnline() { + try { + return callOnline(() -> Engine.getEngine("PyTorch")); + } catch (Exception e) { + logger.error(e.getMessage(), e); + return null; + } + } + /** * Get the available devices for PyTorch, including MPS if Apple Silicon. * @return Only "cpu" if no local engine is found. */ - static Collection getAvailableDevices() { + public static Collection getAvailableDevices() { try { Set availableDevices = new LinkedHashSet<>(); @@ -54,7 +67,7 @@ static Collection getAvailableDevices() { * Query if the PyTorch engine is already available, without a need to download. * @return */ - static boolean hasPyTorchEngine() { + public static boolean hasPyTorchEngine() { return getEngineOffline() != null; } @@ -71,19 +84,6 @@ static Engine getEngineOffline() { } } - /** - * Get the PyTorch engine, downloading if necessary. - * @return the engine if available, or null if this failed - */ - static Engine getEngineOnline() { - try { - return callOnline(() -> Engine.getEngine("PyTorch")); - } catch (Exception e) { - logger.error(e.getMessage(), e); - return null; - } - } - /** * Call a function with the "offline" property set to true (to block automatic downloads). * @param callable diff --git a/src/main/java/qupath/ext/instanseg/core/TilePredictionProcessor.java b/src/main/java/qupath/ext/instanseg/core/TilePredictionProcessor.java index 1893dbe..c0da6a3 100644 --- a/src/main/java/qupath/ext/instanseg/core/TilePredictionProcessor.java +++ b/src/main/java/qupath/ext/instanseg/core/TilePredictionProcessor.java @@ -53,6 +53,14 @@ class TilePredictionProcessor implements Processor { this.doPadding = doPadding; } + /** + * The number of tiles that failed during processing. + * @return The count of the number of failed tiles. + */ + public int nFailed() { + return nFailed; + } + @Override public Mat process(Parameters params) throws IOException { @@ -169,11 +177,5 @@ private static ImageOp getNormalization(ImageData imageData, Path return defaults; } - /** - * The number of tiles that failed during processing. - * @return The count of the number of failed tiles. - */ - public int nFailed() { - return nFailed; - } + } diff --git a/src/main/java/qupath/ext/instanseg/ui/InstanSegController.java b/src/main/java/qupath/ext/instanseg/ui/InstanSegController.java index ac2928f..9da31f2 100644 --- a/src/main/java/qupath/ext/instanseg/ui/InstanSegController.java +++ b/src/main/java/qupath/ext/instanseg/ui/InstanSegController.java @@ -29,6 +29,7 @@ import org.slf4j.LoggerFactory; import qupath.ext.instanseg.core.InstanSeg; import qupath.ext.instanseg.core.InstanSegModel; +import qupath.ext.instanseg.core.PytorchManager; import qupath.fx.dialogs.Dialogs; import qupath.fx.dialogs.FileChoosers; import qupath.fx.utils.FXUtils; @@ -90,18 +91,19 @@ public class InstanSegController extends BorderPane { private CheckBox nucleiOnlyCheckBox; private final ExecutorService pool = Executors.newSingleThreadExecutor(ThreadTools.createThreadFactory("instanseg", true)); - private final QuPathGUI qupath = QuPathGUI.getInstance(); + private final QuPathGUI qupath; private ObjectProperty> pendingTask = new SimpleObjectProperty<>(); private MessageTextHelper messageTextHelper; private final Watcher watcher = new Watcher(modelChoiceBox); private ExecutorService executor; - public static InstanSegController createInstance() throws IOException { - return new InstanSegController(); + public static InstanSegController createInstance(QuPathGUI qupath) throws IOException { + return new InstanSegController(qupath); } - private InstanSegController() throws IOException { + private InstanSegController(QuPathGUI qupath) throws IOException { + this.qupath = qupath; var url = InstanSegController.class.getResource("instanseg_control.fxml"); FXMLLoader loader = new FXMLLoader(url, resources); loader.setRoot(this); @@ -119,6 +121,38 @@ private InstanSegController() throws IOException { configureChannelPicker(); } + + void interrupt() { + watcher.interrupt(); + } + + /** + * Open the model directory in the system file browser when double-clicked. + * @param event + */ + @FXML + void handleModelDirectoryLabelClick(MouseEvent event) { + if (event.getClickCount() != 2) { + return; + } + var path = InstanSegPreferences.modelDirectoryProperty().get(); + if (path == null || path.isEmpty()) { + return; + } + var file = new File(path); + if (file.exists()) { + GuiTools.browseDirectory(file); + } else { + logger.debug("Can't browse directory for {}", file); + } + } + + @FXML + void promptForModelDirectory() { + promptToUpdateDirectory(InstanSegPreferences.modelDirectoryProperty()); + } + + private void configureChannelPicker() { updateChannelPicker(qupath.getImageData()); qupath.imageDataProperty().addListener((v, o, n) -> updateChannelPicker(n)); @@ -155,6 +189,16 @@ private static String getCheckComboBoxText(CheckComboBox comb return String.format(resources.getString("ui.options.nChannelSelected"), n); } + /** + * Add an option to the ContextMenu of the CheckComboBox to select all + * currently-visible channels. + *

+ * Particularly useful for images with many channels - it's possible + * to preview a subset of channels using the brightness and contrast + * window, and to then transfer this selection to InstanSeg by simply + * right-clicking and choosing "Set from visible". + * @param comboChannels The CheckComboBox for selecting channels. + */ private void addSetFromVisible(CheckComboBox comboChannels) { var mi = new MenuItem(); mi.setText("Set from visible"); @@ -231,7 +275,7 @@ private void configureRunning() { qupath.imageDataProperty().isNull() .or(pendingTask.isNotNull()) .or(modelChoiceBox.getSelectionModel().selectedItemProperty().isNull()) - .or(messageTextHelper.warningText().isNotEmpty()) + .or(messageTextHelper.hasWarning()) .or(deviceChoices.getSelectionModel().selectedItemProperty().isNull()) ); pendingTask.addListener((observable, oldValue, newValue) -> { @@ -272,10 +316,6 @@ private static void overrideToggleSelected(ToggleButton button) { button.selectedProperty().addListener((value, oldValue, newValue) -> button.setSelected(false)); } - public void interrupt() { - watcher.interrupt(); - } - private void handleModelDirectory(String n) { if (n == null) return; var path = Path.of(n); @@ -342,7 +382,7 @@ static void addModelsFromPath(String dir, ComboBox box) { } } - public void restart() { + void restart() { executor = Executors.newSingleThreadExecutor(); executor.submit(watcher::processEvents); } @@ -417,6 +457,7 @@ protected Void call() { } qupath.getImageData().getHierarchy().fireHierarchyChangedEvent(this); + String cmd = String.format(""" import qupath.ext.instanseg.core.InstanSeg import static qupath.lib.gui.scripting.QPEx.* @@ -478,31 +519,6 @@ private void selectAllTMACores() { hierarchy.getSelectionModel().setSelectedObjects(hierarchy.getTMAGrid().getTMACoreList(), null); } - /** - * Open the model directory in the system file browser when double-clicked. - * @param event - */ - @FXML - public void handleModelDirectoryLabelClick(MouseEvent event) { - if (event.getClickCount() != 2) { - return; - } - var path = InstanSegPreferences.modelDirectoryProperty().get(); - if (path == null || path.isEmpty()) { - return; - } - var file = new File(path); - if (file.exists()) { - GuiTools.browseDirectory(file); - } else { - logger.debug("Can't browse directory for {}", file); - } - } - - @FXML - public void promptForModelDirectory() { - promptToUpdateDirectory(InstanSegPreferences.modelDirectoryProperty()); - } private void promptToUpdateDirectory(StringProperty dirPath) { var modelDirPath = dirPath.get(); diff --git a/src/main/java/qupath/ext/instanseg/ui/InstanSegExtension.java b/src/main/java/qupath/ext/instanseg/ui/InstanSegExtension.java index 4992704..0c5f9b7 100644 --- a/src/main/java/qupath/ext/instanseg/ui/InstanSegExtension.java +++ b/src/main/java/qupath/ext/instanseg/ui/InstanSegExtension.java @@ -53,16 +53,16 @@ public void installExtension(QuPathGUI qupath) { private void addMenuItem(QuPathGUI qupath) { var menu = qupath.getMenu("Extensions>" + EXTENSION_NAME, true); MenuItem menuItem = new MenuItem("Run InstanSeg"); - menuItem.setOnAction(e -> createStage()); + menuItem.setOnAction(e -> createStage(qupath)); menuItem.disableProperty().bind(enableExtensionProperty.not()); menu.getItems().add(menuItem); } - private void createStage() { + private void createStage(QuPathGUI qupath) { if (stage == null) { try { stage = new Stage(); - var pane = InstanSegController.createInstance(); + var pane = InstanSegController.createInstance(qupath); Scene scene = new Scene(pane); pane.heightProperty().addListener((v, o, n) -> handleStageHeightChange()); stage.setScene(scene); diff --git a/src/main/java/qupath/ext/instanseg/ui/InstanSegPreferences.java b/src/main/java/qupath/ext/instanseg/ui/InstanSegPreferences.java index fe46744..c34c590 100644 --- a/src/main/java/qupath/ext/instanseg/ui/InstanSegPreferences.java +++ b/src/main/java/qupath/ext/instanseg/ui/InstanSegPreferences.java @@ -5,13 +5,12 @@ import javafx.beans.property.StringProperty; import qupath.lib.gui.prefs.PathPrefs; -public class InstanSegPreferences { +class InstanSegPreferences { private InstanSegPreferences() { throw new AssertionError("Cannot instantiate this class"); } - private static final StringProperty modelDirectoryProperty = PathPrefs.createPersistentPreference( "instanseg.model.dir", null); @@ -28,19 +27,19 @@ private InstanSegPreferences() { "intanseg.tile.size", 256); - public static StringProperty modelDirectoryProperty() { + static StringProperty modelDirectoryProperty() { return modelDirectoryProperty; } - public static StringProperty preferredDeviceProperty() { + static StringProperty preferredDeviceProperty() { return preferredDeviceProperty; } - public static Property numThreadsProperty() { + static Property numThreadsProperty() { return numThreadsProperty; } - public static IntegerProperty tileSizeProperty() { + static IntegerProperty tileSizeProperty() { return tileSizeProperty; } } diff --git a/src/main/java/qupath/ext/instanseg/ui/MessageTextHelper.java b/src/main/java/qupath/ext/instanseg/ui/MessageTextHelper.java index b1e0900..097c5ae 100644 --- a/src/main/java/qupath/ext/instanseg/ui/MessageTextHelper.java +++ b/src/main/java/qupath/ext/instanseg/ui/MessageTextHelper.java @@ -124,15 +124,15 @@ private String getWarningText() { return null; } - public StringBinding messageLabelText() { + StringBinding messageLabelText() { return messageLabelText; } - public BooleanBinding hasWarning() { + BooleanBinding hasWarning() { return hasWarning; } - public StringBinding warningText() { + StringBinding warningText() { return warningText; } diff --git a/src/main/java/qupath/ext/instanseg/ui/Watcher.java b/src/main/java/qupath/ext/instanseg/ui/Watcher.java index 967975b..cea0d8f 100644 --- a/src/main/java/qupath/ext/instanseg/ui/Watcher.java +++ b/src/main/java/qupath/ext/instanseg/ui/Watcher.java @@ -1,5 +1,6 @@ package qupath.ext.instanseg.ui; +import javafx.application.Platform; import org.controlsfx.control.SearchableComboBox; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -87,14 +88,18 @@ void processEvents() { logger.debug("{}: {}", event.kind().name(), child); if (kind == ENTRY_CREATE && InstanSegModel.isValidModel(name)) { - try { - modelChoiceBox.getItems().add(InstanSegModel.fromPath(child)); - } catch (IOException e) { - logger.error("Unable to add model", e); - } + Platform.runLater(() -> { + try { + modelChoiceBox.getItems().add(InstanSegModel.fromPath(child)); + } catch (IOException e) { + logger.error("Unable to add model from path", e); + } + }); } if (kind == ENTRY_DELETE && InstanSegModel.isValidModel(name)) { - modelChoiceBox.getItems().removeIf(model -> model.getPath().equals(child)); + Platform.runLater(() -> { + modelChoiceBox.getItems().removeIf(model -> model.getPath().equals(child)); + }); } } diff --git a/src/main/resources/qupath/ext/instanseg/ui/strings.properties b/src/main/resources/qupath/ext/instanseg/ui/strings.properties index c299d62..6129e92 100644 --- a/src/main/resources/qupath/ext/instanseg/ui/strings.properties +++ b/src/main/resources/qupath/ext/instanseg/ui/strings.properties @@ -87,3 +87,4 @@ error.no-imagedata = Cannot run InstanSeg without ImageData. error.downloading = Error downloading files error.localModel = Can't find file in user model directory error.tiles-failed = %d tiles failed. This is often a memory issue.\nConsider decreasing tile size or the number of threads used. +error.modelPath = Unable to fetch model path