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

Add tables weight-formats and pre-post-processing #43

Merged
merged 26 commits into from
Nov 26, 2020
Merged

Conversation

constantinpape
Copy link
Collaborator

No description provided.

@constantinpape
Copy link
Collaborator Author

@oeway This would be my suggestion for listing the supported weight formats, pre-processing, and post-processing, as well as listing which consumer supports which option.

Let me know what you think. Once we agree on a format I will try to fill the lists for pre/post-processing and ask others to double check.

@oeway
Copy link
Contributor

oeway commented Nov 13, 2020

Thanks! Looks good to me! In addition to yes and no, there might be partial implementations, and we can add a footnote below the table.

@constantinpape
Copy link
Collaborator Author

Thanks! Looks good to me!

Ok, I will finish up everything as far as I can and then ask the others to fill in the gaps.

there might be partial implementations, and we can add a footnote below the table.

Good point; we can add this if it becomes relevant.

@constantinpape constantinpape changed the title WIP: Add tables weight-formats and pre-post-processing Add tables weight-formats and pre-post-processing Nov 13, 2020
@constantinpape
Copy link
Collaborator Author

I gathered all we had for pre/post-processing in #32, #37 and #39.
Let me know if you think anything is missing or you want any other changes here, @oeway @esgomezm @frauzufall @FynnBe @m-novikov.

@constantinpape constantinpape mentioned this pull request Nov 17, 2020
@esgomezm
Copy link
Contributor

It looks very good! thank you

I might have missed it, sorry, what is reference_implementation?

For the post-processing, should we allow basic thresholding? something like:

- `intensity_threshold` binarize the intensity values of an image by thresholding them with a specific value. 
  - `kwargs`
    - `threshold` For example [0.5, 75, 0.25] in the case of a 3 channel image where the channels are not thresholded jointly
  - `reference_implementation`

For the pre/post-processing support in @deepimagej we have kind of a solution but it's not clear for us whether it can be considered as "supported processing": both steps are given in IJ macros, so as long as they are not super complicated workflows, we are able to reproduce most of the defined pre/post processings. The problem is that we are not really reading the transformations from the yaml, but from the macros. (Comments are very welcome)

@constantinpape
Copy link
Collaborator Author

I might have missed it, sorry, what is reference_implementation?

My idea was to provide a link to the reference implementation for the given pre/post-processing, to make it easier to reimplement it. (I haven't provided any links yet, we can fill that in as we go.)

For the post-processing, should we allow basic thresholding? something like:

I think providing basic thresholding as post-processing is a good idea.

both steps are given in IJ macros, so as long as they are not super complicated workflows, we are able to reproduce most of the defined pre/post processings.

Are the macros provided separately by the users?
Do you think you could implement a system where you have default macros corresponding to the pre/postprocessing operations here (at least to the ones that you want to support) and select them according to the yaml? (The user could still provide different macros but at least you could default to what's specified in the yaml).

@esgomezm
Copy link
Contributor

My idea was to provide a link to the reference implementation for the given pre/post-processing, to make it easier to reimplement it. (I haven't provided any links yet, we can fill that in as we go.)

Good!

Are the macros provided separately by the users?

Yes.

Do you think you could implement a system where you have default macros corresponding to the pre/postprocessing operations here (at least to the ones that you want to support) and select them according to the yaml? (The user could still provide different macros but at least you could default to what's specified in the yaml).

Yes but not really at this moment. What we have is a GitHub repo containing processing routines (macros), so the users can (manually) download and include them in the model package. Here, we can provide the ones specified as "bioimage.io macros" that are exactly what it is defined here. With the example input/output images, it is possible to know whether the model is working properly on different consumers, but still...

@constantinpape
Copy link
Collaborator Author

Yes but not really at this moment. What we have is a GitHub repo containing processing routines (macros), so the users can (manually) download and include them in the model package. Here, we can provide the ones specified as "bioimage.io macros" that are exactly what it is defined here.

Ok, then I would suggest that you mark the ones where you have a corresponding macro with yes (and you can also provide the link to macro). And we can then add a footnote that explains that the pre/post-processing currently needs to be selected manually in deepImageJ.

@frauzufall
Copy link
Contributor

Can we add a 'binarize' postprocessing step with threshold as an argument (or something similar)?

@frauzufall
Copy link
Contributor

@constantinpape the lower and upper percentile can be between 0 - 100, correct? Should we add this as a note?

@constantinpape
Copy link
Collaborator Author

@constantinpape the lower and upper percentile can be between 0 - 100, correct? Should we add this as a note?

Yes, good point.

@FynnBe
Copy link
Member

FynnBe commented Nov 25, 2020

Would it be weird to disallow the upper percentile to be <1 as a sanity check? Of course it's a valid value, but someone might specify the percentiles in 0,1... and we wouldn't catch it

Co-authored-by: FynnBe <thefynnbe@gmail.com>
constantinpape and others added 10 commits November 25, 2020 17:44
Co-authored-by: FynnBe <thefynnbe@gmail.com>
Co-authored-by: FynnBe <thefynnbe@gmail.com>
Co-authored-by: FynnBe <thefynnbe@gmail.com>
pointing to pytorch-bioimage-io instead, where comments have been added;
avoiding to maintain the same model twice.
@frauzufall
Copy link
Contributor

Can I add one more request? 🙏 We use clip also in postprocessing - exactly as defined in preprocessing.

@constantinpape
Copy link
Collaborator Author

Can I add one more request? pray We use clip also in postprocessing - exactly as defined in preprocessing.

Sure! I will add it later and then would merge this PR. We can fill in what's missing in later PR, this one is becoming pretty long already...

Any objections?

@frauzufall
Copy link
Contributor

Also, percentile preprocessing uses lower_percentile and upper_percentile, whereas in postprocessing scale_min_max uses min_percentile and max_percentile. And this postprocessing does not work without a reference to the input node - without the reference, min and max percentile cannot be calculated from the normalized output image. But maybe that's ok. I implemented it now like this in our consumer and it works, it can be improved later.

@constantinpape
Copy link
Collaborator Author

lower_percentile and upper_percentile, whereas in postprocessing scale_min_max uses min_percentile and max_percentile

Ok, we should use the same name here, I will change it to min/max in the preprocessing.

And this postprocessing does not work without a reference to the input node - without the reference, min and max percentile cannot be calculated from the normalized output image. But maybe that's ok. I implemented it now like this in our consumer and it works, it can be improved later.

Do you mean that the mode fixed for scale_min_max (where you would specify a fixed value for min and max) is not possible?
Anyway, I agree maybe better to discuss this in the call tomorrow and fix it later.

@constantinpape constantinpape merged commit 2e9de01 into master Nov 26, 2020
@frauzufall
Copy link
Contributor

@constantinpape yes I see that the perceniles in scale_min_max already has the note that it does not work for fixed. So that's all good. Thanks for the prompt answers!

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

Successfully merging this pull request may close these issues.

5 participants