Download models #54

alanocallaghan · 2024-08-28T17:52:07Z

Enable downloading models. Checks the most recent release for zip files and adds these to the model search box. Remote models are downloaded and unzipped on request into the user local directory.

Local models now go in: [my-model-directory]/local, while downloaded models go in [my-model-directory]/ directly.
Doesn't overwrite any existing files for the time being (in future it should check for content/checksums). Should show already-downloaded models as downloaded without needing to hit the download button to check.

Adds README popover to icon button

Resolve #46

petebankhead

I wasn't able to get this working

I had an exception first because I didn't have the directory
When I click download, I get q.l.g.QuPathUncaughtExceptionHandler - Cannot invoke "qupath.ext.instanseg.core.InstanSegModel.getName()" because "model" is null... and if I choose a different model, the download option disappears.

I think some of our changes conflict - especially around pixel sizes (although I think I introduced an inconsistency when I switched to downsample with DetectionMeasurer and forgot to update the name...). It's important that we try to ensure a consistent downsample is used, and it is always calculated from the model spec... unless the user overrides it in a script.

From reading the code, it looks to me like it will always query the available models online. If that's right, it makes me uneasy; I feel the user should be explicitly requesting any check, and it should not happen automatically.

petebankhead · 2024-09-04T17:59:00Z

src/main/java/qupath/ext/instanseg/ui/InstanSegController.java

@@ -352,8 +466,9 @@ private void handleModelDirectory(String n) {
        var path = Path.of(n);
        if (Files.exists(path) && Files.isDirectory(path)) {
            try {
-                watcher.register(path); // todo: unregister
-                addModelsFromPath(n, modelChoiceBox);
+                var localPath = path.resolve("local");


I get a java.nio.file.NoSuchFileException: here the first time I run it.

petebankhead · 2024-09-04T18:01:41Z

src/main/java/qupath/ext/instanseg/ui/InstanSegController.java

            var instanSeg = InstanSeg.builder()
                    .model(model)
                    .imageData(imageData)
                    .device(deviceChoices.getSelectionModel().getSelectedItem())
                    .numOutputChannels(nucleiOnlyCheckBox.isSelected() ? 1 : 2)
                    .channels(channels.stream().map(ChannelSelectItem::getTransform).toList())
                    .tileDims(InstanSegPreferences.tileSizeProperty().get())
-//                    .outputAnnotations()
+                    .downsample(pixelSize.get().doubleValue() / (double) server.getPixelCalibration().getAveragedPixelSize())


This shouldn't be here. I've tried to remove setting the downsample explicitly - it results in scripts that don't work properly when applied to images with a different resolution.

I didn't remove downsample as an option entirely because it can be handy to adjust in a script, but routinely it shouldn't be set so that elsewhere the model spec can be queried - and a (possibly-rounded) downsample value used instead.

petebankhead · 2024-09-04T18:03:51Z

src/main/java/qupath/ext/instanseg/ui/InstanSegController.java

                            deviceChoices.getSelectionModel().getSelectedItem(),
                            nucleiOnlyCheckBox.isSelected() ? 1 : 2,
                            ChannelSelectItem.toConstructorString(channels),
                            InstanSegPreferences.tileSizeProperty().get(),
+                            pixelSize.get().doubleValue() / (double) server.getPixelCalibration().getAveragedPixelSize(),


Not entirely where where we are in the code (checking on GitHub) but I don't think this should be here. See the use of getPreferredDownsample that I introduced to reduce inconsistencies, and help ensure scripts don't end up being locked to a single resolution by encoding a fixed downsample value.

petebankhead · 2024-09-04T18:05:33Z

src/main/java/qupath/ext/instanseg/core/InstanSegModel.java

@@ -83,15 +136,15 @@ private Number getPixelSizeY() {
     * Otherwise, it is the value of whichever value is available - or null if neither is found.


Update javadocs if you want to switch to Optional.

petebankhead · 2024-09-04T18:07:42Z

src/main/java/qupath/ext/instanseg/core/InstanSegModel.java

     * @return the pixel size in the X dimension.
     */
-    private Number getPixelSizeX() {
-        return getPixelSize().getOrDefault("x", null);
+    public Optional<Number> getPixelSizeX() {


I had made these private - I'm not sure why they are ever needed to be public? We can't handle resizing differently in x and y anyway, and introduced getPreferredDownsample as a method to consistently get the value we are most likely to need.

I'd rather these were private unless there's a good reason, since it increases the likelihood that we forget to use getPreferredDownsample (or getPreferredPixelSize).

alanocallaghan · 2024-09-04T18:24:37Z

When I click download, I get q.l.g.QuPathUncaughtExceptionHandler - Cannot invoke "qupath.ext.instanseg.core.InstanSegModel.getName()" because "model" is null... and if I choose a different model, the download option disappears.

was this with no model selected, or?

alanocallaghan · 2024-09-04T18:30:31Z

From reading the code, it looks to me like it will always query the available models online. If that's right, it makes me uneasy; I feel the user should be explicitly requesting any check, and it should not happen automatically.

I was mostly copying the logic from wsinfer, albeit without saving the "zoo" json locally. Do realise this means that if you download models, then run the extension offline, you won't see any available models because we're only scanning the "local" directory for existing models, and the downloaded ones are identified from the GitHub curl query

Objection to me doing as with wsinfer and caching the JSON response?

I suspect the downsample stuff is all just failed merges.

petebankhead · 2024-09-04T18:42:05Z

Not entirely sure I understand, but don't see an issue with caching the json.

What happens if models are updated, so that the local ones no longer match? Or do we assume that models are permanent and not updated...?

For reproducibility, not updating seems wise. But in the early days, I can imagine that might not happen.

alanocallaghan · 2024-09-04T19:14:13Z

What happens if models are updated, so that the local ones no longer match? Or do we assume that models are permanent and not updated...?

There's currently no versioning. New versions of a model would be skipped, because it just checks if the model exists on disk.

Questions then arise on how we handle versions: overwrite? Present all possible versions (distinguished by...?) etc. Similar headaches to WSInfer

petebankhead · 2024-09-04T19:17:08Z

I think that's ok, the more important thing is not to update automatically.

There were not checks to ensure that `InstanSegPreferences.modelDirectoryProperty().get()` didn't return null, and therefore lots of `Path.of` calls would fail if it was null.

Padding is currently 80 and we subtract 2 x padding from tile size to get the valid area. So 512 is a better default than 256, which has a tiny valid area.

petebankhead · 2024-09-05T05:52:22Z

I was still having trouble with this, but I think they are mostly resolved - please check out my commits.

With this kind of change, testing with 'Edit → Reset preferences' is important to see how it will be with a new installation. This gave lots of errors because of unchecked calls to Path.of(null) when the model directory wasn't set.

I'll merge this now but plan to work more on it later this morning.

alanocallaghan added 7 commits August 28, 2024 18:11

Implementation of downloading alongside local models

97f89d1

Remove todos

f2b9be6

Use optional instead of exceptions

5a3b85b

Use optional for README and re-order methods

77f5191

Merge remote-tracking branch 'origin/main' into download-models

b9ca94a

Remove redundant bool

98f9283

Replace file.exists with dummy method calls

be67023

This comment was marked as outdated.

Sign in to view

alanocallaghan added 2 commits September 4, 2024 16:15

Detect objects in script

9378900

Merge remote-tracking branch 'origin/main' into download-models

814d2d5

alanocallaghan marked this pull request as ready for review September 4, 2024 17:21

Use downsample instead of pixelSize

df06e63

petebankhead requested changes Sep 4, 2024

View reviewed changes

alanocallaghan added 2 commits September 4, 2024 19:56

Cache release information locally

2d513a7

Cache releases JSON

b42cc8d

Merge remote-tracking branch 'origin/main' into download-models

be25e17

alanocallaghan and others added 6 commits September 4, 2024 23:14

Merge remote-tracking branch 'origin/main' into download-models

46354eb

Fix BF models, pretty-print JSON

d83884b

Fix watcher

b766c1d

Fix most exceptions

68fe859

There were not checks to ensure that `InstanSegPreferences.modelDirectoryProperty().get()` didn't return null, and therefore lots of `Path.of` calls would fail if it was null.

Highlight when model directory not set

cb6de7a

Improve tile size & device defaults

8c3e32d

Padding is currently 80 and we subtract 2 x padding from tile size to get the valid area. So 512 is a better default than 256, which has a tiny valid area.

petebankhead approved these changes Sep 5, 2024

View reviewed changes

petebankhead merged commit 8e8d05b into main Sep 5, 2024
1 check passed

petebankhead deleted the download-models branch September 5, 2024 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download models #54

Download models #54

alanocallaghan commented Aug 28, 2024 •

edited

Loading

This comment was marked as outdated.

petebankhead left a comment

petebankhead Sep 4, 2024

petebankhead Sep 4, 2024

petebankhead Sep 4, 2024

petebankhead Sep 4, 2024

petebankhead Sep 4, 2024

alanocallaghan commented Sep 4, 2024

alanocallaghan commented Sep 4, 2024 •

edited

Loading

petebankhead commented Sep 4, 2024

alanocallaghan commented Sep 4, 2024

petebankhead commented Sep 4, 2024

petebankhead commented Sep 5, 2024

		@@ -83,15 +136,15 @@ private Number getPixelSizeY() {
		* Otherwise, it is the value of whichever value is available - or null if neither is found.

Download models #54

Download models #54

Conversation

alanocallaghan commented Aug 28, 2024 • edited Loading

This comment was marked as outdated.

petebankhead left a comment

Choose a reason for hiding this comment

petebankhead Sep 4, 2024

Choose a reason for hiding this comment

petebankhead Sep 4, 2024

Choose a reason for hiding this comment

petebankhead Sep 4, 2024

Choose a reason for hiding this comment

petebankhead Sep 4, 2024

Choose a reason for hiding this comment

petebankhead Sep 4, 2024

Choose a reason for hiding this comment

alanocallaghan commented Sep 4, 2024

alanocallaghan commented Sep 4, 2024 • edited Loading

petebankhead commented Sep 4, 2024

alanocallaghan commented Sep 4, 2024

petebankhead commented Sep 4, 2024

petebankhead commented Sep 5, 2024

alanocallaghan commented Aug 28, 2024 •

edited

Loading

alanocallaghan commented Sep 4, 2024 •

edited

Loading