Implement result export

[constantinpape]

See how we can use the writer io hook to export the annotation results.

[constantinpape]

Initial functionality implemented in 139d682.
I did not use the API hooks, but instead went with Shift + S and a button in the GUI.

Still need to implement the logic for the partial annotations:

• Save partially annotated to _partial_annotations.h5 instead of _annotations.h5.
• Enable setting the is_partial flag from the gui.
• Force is_partial if not all cells have been annotated.
• Warn user if annotations are partial.

[sofroniewn]

I did not use the API hooks, but instead went with Shift + S and a button in the GUI.

Would be great to know why not/ if there is something we could do to make the current API hooks better, or if there are different hooks that you would have preferred. I'll note that right now there is no hook napari_write_viewer which passes all the viewer information too. Would that have helped? Looking through your commit it does look like you could have used the napari_get_writer hook.

Feedback welcome, you're one of our first real world test cases!!

[constantinpape]

@sofroniewn
Yes, I tried to use the napari_get_writer, but decided not to use it:

1. I could not get it to work as expected.
2. I think it's not a good fit for saving results, because I only want to save the labels layer (and the infected labels, but they are in the labels layer's metadata), so it felt kind of unnecessary to have users go to Files->Save Selected Layers (where one would need to make sure that the labels layer is actually selected) / Files->Save All Layers.

Re 1.: I will give a more detailed summary of what went wrong tomorrow (it's a bit late in Germany already ;)). But in general having a full example for the napari_get_writer would help a lot. I did not find it straight forward to adapt the examples for napari_get_reader.

Re 2. Yes, napari_write_viewer might be helpful here, because I would just want to get the whole state and then serialize the necessary parts.

[constantinpape]

Ok, I can demonstrate that napari_get_writer is not working as I would have expected:

I try to save the labels layer as h5:

But it can't find a writer:

I would have expected that my hook implementation is picked up for this case.
Looking at it again, I realize that it might be due to this check. But the problem is that the exact arguments passed in layers are not specified in the documentation. I just assumed that it would be the same as the return values of the function returned by napari_get_reader.

[sofroniewn]

I could not get it to work as expected.

Ok good to know you struggled with napari_get_writer - we might want to add a napari_write_layers hookspec that doesn't require you to jump through the napari_get_writer hook, and eventually deprecated napari_get_writer if it is too confusing. I know I had advocated for it @tlambert03, but maybe we should revisit.

because I only want to save the labels layer (and the infected labels, but they are in the labels layer's metadata), so it felt kind of unnecessary to have users go to Files->Save Selected Layers (where one would need to make sure that the labels layer is actually selected)

Hmm ok, here I would have hoped that the napari_write_labels hookspec would have worked for you in this case. Maybe now with napari/napari#1284 merged it will be more clear what layer is selected, and so the work flow would be more intuitive.

Anyway, whatever you choose if fine for now, but this is helpful for us as we think about the plugin interface and workflows

[tlambert03]

I actually see this more as a keyboard shortcut issue: @constantinpape would be fine if ctrl-S mapped to Save All Layers, rather than Save Selected Layer(s). He could then write a napari_get_writer function would just ignore everything he didn't care about and only saved the one Labels layer he wanted.

I'd also say this is a slightly unusual plugin scenario (it's more of a sub-app, as has already been pointed out). In most cases, i don't think we're going to want plugins to be able to completely own the ctrl-S behavior such that, regardless of what the user has selected, everything but a single Labels layer is ignored.

[tlambert03]

just to be clear here, it's this line that is the problem. Something like this should work

# NOTE this doesn't work yet, but also doesn'make much sense in this context
@napari_hook_implementation
def napari_get_writer(path, layers):
    
    # make sure there is at least one labels
    if any(l == 'labels' for l in layers):
        return save_labels

...and then you need a save_labels function that looks through the layer data for the layer you care about. We might even be able to help you monkey patch the keyboard shortcut

[constantinpape]

I actually see this more as a keyboard shortcut issue: @constantinpape would be fine if ctrl-S mapped to Save All Layers, rather than Save Selected Layer(s). He could then write a napari_get_writer function would just ignore everything he didn't care about and only saved the one Labels layer he wanted.

Yes, this makes sense.

I'd also say this is a slightly unusual plugin scenario (it's more of a sub-app, as has already been pointed out). In most cases, i don't think we're going to want plugins to be able to completely own the ctrl-S behavior such that, regardless of what the user has selected, everything but a single Labels layer is ignored.

I also agree. I think the clean way would be to redesign this project so that it is a proper plugin, don't hard-codes the layer names but offers the extra functionality as layer decorators.
Then the save function would need to somehow check (maybe through additional metadata) if the layer has been decorated and then save it differently.

I don't have time for this now, and I also don't think napari plugins support all we need for this approach yet, but I would be very interested to keep working on this once things calm down.

[constantinpape]

just to be clear here, it's this line that is the problem. Something like this should work

I am not sure. I changed to your suggestion now:
https://github.com/hci-unihd/covid-if-annotations/blob/master/napari_covid_if_annotations/io_hooks.py#L25-L38

But it still doesn't work. Also the "Hook called" is never printed, so it looks to me like the napari_get_writer is not called. But I don't quite understand where this happens, so maybe we don't expect stdio to work in the hooks.

[constantinpape]

We will not do the partial results functionality in the app, but rather as post-processing.
So all functionality is implemented now.
I am gonna leave this open for the time being to discuss the issues with the writer hook as feedback for the napari plugin desing.