Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ideas for dealing with multiple data types #1

Open
MosGeo opened this issue Oct 28, 2021 · 10 comments
Open

Ideas for dealing with multiple data types #1

MosGeo opened this issue Oct 28, 2021 · 10 comments

Comments

@MosGeo
Copy link

MosGeo commented Oct 28, 2021

I am starting to work on this functionality for my workflows. Initially, I was thinking of starting from your code and expanding on it and contribute it back and at the same time use it for myself. Now I hitting the first hurdle so I thought to get your opinion.

  1. How should we deal with different data, specifically, I am trying to unify my workflows around dask arrays coming from aicsimageio. So the format for it is TCXYZS. I am not sure If we can generalize the cropping functionality to this format and the normal order of things that you have right now.

  2. For dealing with RGB images, can we assume that the last index is a potential RGB, check if it is 3 and check "Is RGB" boolean by default. The user would be able to uncheck it. If it is checked, it would be easy to exclude it from the cropping.

@haesleinhuepf
Copy link
Member

Hi @MosGeo ,

I must admit, I also don't know how to solve these issues. I have the feeling it's an unsolved riddle in python and napari. As long as the tool still works for single channel 2D and 3D data, I will merge all pull-requests you send 🙂

@haesleinhuepf
Copy link
Member

Hi Mustafa @MosGeo ,

we have just released version 0.1.4 which now supports RGB images. Would you mind checking if it works for your use-case? Furthermore, I was wondering: You mentioned TCXYZS, what does the S stand for?

Thanks!
Robert

@tdmorello
Copy link
Contributor

Hey Robert @haesleinhuepf

I think S stands for scene.

@tdmorello
Copy link
Contributor

tdmorello commented Jan 18, 2022

I am making a note that dask arrays do not support all numpy slicing syntax. We use slicing with tuples in the current code, therefore, we will probably have to refactor.

My first thought (especially for simplicity) is to crop in a for loop. Something like this:

for ax in axes:
  # find min and max of shape coordinates along that axis
  # if min and max are the same, do not crop, copy the entire axis
  # otherwise crop according to coordinates

If we move into larger datasets, we should consider setting up benchmarks to optimize slicing algorithms!

@MosGeo
Copy link
Author

MosGeo commented May 25, 2022

@haesleinhuepf @tdmorello

So I just made something I have been working on public. It has Lots of features (cropping and masking, regular and irregular) Can you please check it out and let me know what you think. I still have not published it in pypi. Honestly, I don't want napari to have lots of "cropping" tools but I felt that my use case is different that I needed to start from scratch.

https://github.com/MosGeo/napari-crop-and-mask

  1. The core cropping and masking algorithms are dask based.
  2. It should be able to deal with nd images (currently cropping in the xy).
  3. Works with RGB.
  4. The algorithm and interface (widget) are separated in code.
  5. Technically, the core algorithms can find a place somewhere, e.g., dask-image, rather than in the widget. I am not sure if they are the best implementation though as I am not a dask expert.
  6. I am not using MagicGui right now. I am currently more comfortable with plain old PySide/PyQt. For example, maguicgui-napari still does not include automatic renaming of layers in drop down.

Might create some gifs later for illustrations of some of the features. I really like the irregular masking feature :)

p.s. napari still does not like RGB images with nan values but cropping/masking works in terms of algorithms.

image

@zoccoler
Copy link
Contributor

Hi @MosGeo ,

Thanks for reaching out here to discuss these different implementations! I am also in favor of not having "repeated" tools here and there, especially in a same platform.

So I gave it a quick trial in your plugin and it seems to work really well! It brings more functionalities that we have not implemented here yet, but are in our to-do list, like dask and in-place crop.

I have two major comments that I list below, but overall I think, if you like and others too, we could reach an agreement to merge both implementations in a single thing and of course have you as author as well! So here comes my comments:

  1. A difference I noticed is that the results from your plugin are kept with the same shape as the original image, while here we have made them as minimal bounding boxes. They could be positioned back to the original image place with the translate option of layers for example. Is there a specific need for you to keep the original shape? This could be an option too!

  2. I think the main interface is a bit intimidating for a simple crop plugin. I am usually in favor of keeping it as minimal as possible because most use cases would just go for the Run button, right? A suggestion that comes to mind would be to have most options as a collapsible tab, what do you think? btw, I am totally ok with having just plain PyQt :)

Technically, the core algorithms can find a place somewhere, e.g., dask-image, rather than in the widget.

Sorry, I did not understand what you meant here.

Let me know where you see these plugins going!

Best,
Marcelo

@MosGeo
Copy link
Author

MosGeo commented May 29, 2022

@zoccoler: Thanks!

Inplace vs transform

  • Cropping: you should have the option to do in-place crop (with transform like you suggested) or just normal crop (keeping the minimal bounding box). Actually, that is what the in-place crop button is for. But I see that I forgot to implement it (right now, it just prints that it was selected 🤣). Right now, cropping always defaults to minimal bounding box. I will implement it
  • Masking: as it is masking, it is always going to just mask. So, there will be no minimal bounding box.

Confusing and intimidating interface:

  • I agree that it is intimidating and confusing especially since some options are now only for cropping (e.g., in-place crop). I am thinking now to split it into two widgets: crop and mask. They can be installed from one extension. What do you think?
  • I also like the collapsible option. Actually, I am the one who first implemented QCollapsible in superqt for these kinds of scenarios. I just didn’t use it here yet. Combined with two widgets, we can simplify the interface considerably.

Core algorithms:

The core.py file contains algorithms that does not rely on napari. It is just dask operations. So I am thinking this files (actually, a more organized one with better implementation) should be setting in another repo (dask-image https://github.com/dask/dask-image. @jni What do you think?

Where I see this is going:

  • Performance issue: I suspect that a lot of people will not need dask for image manipulation. So they are taking a performance hit if they use it.
  • Backend issues: if cropping returns a dask array and their subsequent workflows expects numpy array, they might face some issues. For me, I've decided early on to rely on dask as a backend for my workflows (starting with image loading using aicsimageio)
  • Repo: I have no issues of my code being absorbed by this repo (in fact, in a lot of ways, I prefer it; like I said, I prefer to have one good tool than two adequate ones). Also, the, a lab would be able to 'support' it than one person (me). The performance and backend are the reasons why I just didn't just make pull requests to this repo. Dealing with dask array can be tricky so I had to rely on some things that you don't need to do in numpy to make it work. I also need all these different features that I already implemented in my workflows (e.g., irregular masking).

Do you think the two implementations should be merged to one? Do you see a path for that in terms of code (numpy vs dask)?

@zoccoler
Copy link
Contributor

Cropping, masking and interface

I see! Masking and cropping could indeed be separated.

Actually, I am the one who first implemented QCollapsible in superqt for these kinds of scenarios.

That's awesome! I think it would fit right in!

Core algorithms and dask

I think for now they could be kept here. If they are really generalizable and a feature of interest for dask, we could open an issue there and discuss.

About whether to use dask or not, I think that could either be an option the user can set or an early check around the input (if it is a daskarray, process it as such, otherwise, do conventional numpy slicing). What do you think?

Single repo

I have no issues of my code being absorbed by this repo (in fact, in a lot of ways, I prefer it; like I said, I prefer to have one good tool than two adequate ones). Also, the, a lab would be able to 'support' it than one person (me).

That would be cool and you could be a co-maintainer. Here, one main function does all the job for now. Maybe your interface would "just" need to call it (something similar to this line). Notice that the function here already does irregular cropping (not masking though) and that there is at least one user relying on it directly from code, so I would be careful about changing function names and input/output variables unless really necessary.

@MosGeo
Copy link
Author

MosGeo commented Jul 24, 2022

@zoccoler so I modified things to make it clearer. Now, I have two widgets, one for cropping and one for masking. Advanced options are in QCollapsible so the two widgets are very clean now.

  • Cropping: Always rectangular. It allows for both normal cropping or cropping with translation to preserve the location.
  • Masking: Always returns the image of the same shape as the input. It could be any number of irregular shapes or you can force it to rectangular. The mask value could be nan or zero. And you can exclude or include the selected shapes.

@zoccoler
Copy link
Contributor

Hi @MosGeo ,

Cool! I just gave it a try and I love the new interface! In my opinion, the collapsible menus indeed made the interface much cleaner!

How do you intend to proceed from here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants