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 deepimagej config generation #151

Merged
merged 19 commits into from
Nov 28, 2021
Merged

Add deepimagej config generation #151

merged 19 commits into from
Nov 28, 2021

Conversation

constantinpape
Copy link
Contributor

Write the deepimagej config. Still need to get the pixel size information somehow.
cc @esgomezm

@github-actions github-actions bot added the enhancement New feature or request label Nov 20, 2021
bioimageio/core/build_spec/build_model.py Outdated Show resolved Hide resolved
bioimageio/core/build_spec/build_model.py Outdated Show resolved Hide resolved
bioimageio/core/build_spec/build_model.py Outdated Show resolved Hide resolved
bioimageio/core/build_spec/build_model.py Show resolved Hide resolved
bioimageio/core/build_spec/build_model.py Outdated Show resolved Hide resolved
bioimageio/core/build_spec/build_model.py Outdated Show resolved Hide resolved
bioimageio/core/build_spec/build_model.py Outdated Show resolved Hide resolved
bioimageio/core/build_spec/build_model.py Outdated Show resolved Hide resolved
tests/build_spec/test_build_spec.py Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@esgomezm
Copy link
Contributor

Hi @constantinpape

In the ZeroCostDL4Mic notebook for StarDist we directly ask the user for this value. By default the pixel size could be set to 1 pixel.

In the rest of the notebooks, it's calculated using the following function:

from PIL import Image 
def getPixelSizeTIFFmetadata(TIFFpath, display=False):
  with Image.open(TIFFpath) as img:
    meta_dict = {TAGS[key] : img.tag[key] for key in img.tag.keys()}


  # TIFF tags
  # https://www.loc.gov/preservation/digital/formats/content/tiff_tags.shtml
  # https://www.awaresystems.be/imaging/tiff/tifftags/resolutionunit.html
  ResolutionUnit = meta_dict['ResolutionUnit'][0] # unit of resolution
  width = meta_dict['ImageWidth'][0]
  height = meta_dict['ImageLength'][0]

  xResolution = meta_dict['XResolution'][0] # number of pixels / ResolutionUnit

  if len(xResolution) == 1:
    xResolution = xResolution[0]
  elif len(xResolution) == 2:
    xResolution = xResolution[0]/xResolution[1]
  else:
    print('Image resolution not defined.')
    xResolution = 1

  if ResolutionUnit == 2:
    # Units given are in inches
    pixel_size = 0.025*1e9/xResolution
  elif ResolutionUnit == 3:
    # Units given are in cm
    pixel_size = 0.01*1e9/xResolution
  else: 
    # ResolutionUnit is therefore 1
    print('Resolution unit not defined. Assuming: um')
    pixel_size = 1e3/xResolution

  if display:
    print('Pixel size obtained from metadata: '+str(pixel_size)+' nm')
    print('Image size: '+str(width)+'x'+str(height))
  
  return (pixel_size, width, height)

@constantinpape
Copy link
Contributor Author

Thanks for sharing the script @esgomezm, but I fear that this is not very useful for us because we can't rely on having tif files as inputs; the (mandatory) input data are numpy arrays here.
I will go ahead and just add the pixel size as a parameter to the build_model function and propagate it to the deepimagej config; then you can pass it to the function.

@FynnBe
Copy link
Member

FynnBe commented Nov 25, 2021

incorporating pixel size into the spec (0.4.?) sounds like another hackathon item to me...

@constantinpape
Copy link
Contributor Author

incorporating pixel size into the spec (0.4.?) sounds like another hackathon item to me...

Yes, def. a point to discuss

@constantinpape constantinpape marked this pull request as ready for review November 25, 2021 23:09
bioimageio/core/build_spec/build_model.py Outdated Show resolved Hide resolved
tests/build_spec/test_build_spec.py Outdated Show resolved Hide resolved
@constantinpape
Copy link
Contributor Author

constantinpape commented Nov 25, 2021

Ok, I updated the deepimagej config function to include the pixel size and fix several issues.
There are still a couple open points:

  • I don't understand the deepimagej binarize macro and how to set the threshold in it @esgomezm
  • Does deepimagej support models with multiple inputs / outptus? If yes, how to specify the pre-post-processing for this in the config? Afaik there is just a single list of pre/postprocessing, which implies that this is applied to a single in/out tensors

Also, the tests are still failing due to some issues with sample inputs / outputs that I don't understand. But this may be fixed by #164, so we should merge that PR first and then merge main here.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@FynnBe
Copy link
Member

FynnBe commented Nov 26, 2021

Also, the tests are still failing due to some issues with sample inputs / outputs that I don't understand. But this may be fixed by #164, so we should merge that PR first and then merge main here.

yes, this should be fixed by #164

@constantinpape
Copy link
Contributor Author

@FynnBe the tests for writing the deepimagej config is still failing with

 marshmallow.exceptions.ValidationError: {'sample_outputs': {0: ['Errors in all options for this field. Fix any of the following errors:', ['/tmp/bioimageio_cache/extracted_packages/8cfc68d44ac8c3a154290017d4151113277ca8a27e72a15b39fb4a0792b0ec79/sample_output_0.
tif'], ['expected relative path.']]}, 'sample_inputs': {0: ['Errors in all options for this field. Fix any of the following errors:', ['/tmp/bioimageio_cache/extracted_packages/8cfc68d44ac8c3a154290017d4151113277ca8a27e72a15b39fb4a0792b0ec79/sample_input_0.tif'], ['expected
 relative path.']]}} 

although sample_inputs/outputs are treated exactly the same as test_inputs/outputs in build_model.

Copy link
Member

@FynnBe FynnBe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost there 👍

bioimageio/core/build_spec/build_model.py Show resolved Hide resolved
bioimageio/core/build_spec/build_model.py Show resolved Hide resolved
# (Pdb) model.test_inputs
# [PosixPath('/tmp/bioimageio_cache/extracted_packages/efae734f36ff8634977c9174f01c854c4e2cfc799bc92751a4558a0edd963da6/test_input.npy')]
# (Pdb) model.test_outputs
# [PosixPath('/tmp/bioimageio_cache/extracted_packages/efae734f36ff8634977c9174f01c854c4e2cfc799bc92751a4558a0edd963da6/test_output.npy')]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fixed by #164 ; then this should be removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tests for writing the deepimagej config is still failing with

... or not.. looking into it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is that sample inputs/outputs are absolute and not relative paths...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but test inputs / outptus are also absolute paths. Why does it work for them and not for sample inputs/outputs?

@FynnBe
Copy link
Member

FynnBe commented Nov 26, 2021

1942df0 contains bugs! not ready yet...

@constantinpape
Copy link
Contributor Author

  • I don't understand the deepimagej binarize macro and how to set the threshold in it @esgomezm

It's optimal_thresold.

  • Does deepimagej support models with multiple inputs / outptus? If yes, how to specify the pre-post-processing for this in the config? Afaik there is just a single list of pre/postprocessing, which implies that this is applied to a single in/out tensors

It's in theory possible, but too complex for now; we will just throw an error for now and leave this for future iterations.

@FynnBe
Copy link
Member

FynnBe commented Nov 26, 2021

locally for me there remains an issue with sample_inputs/outputs serialization. Not sure what's going on there...

@FynnBe
Copy link
Member

FynnBe commented Nov 26, 2021

although sample_inputs/outputs are treated exactly the same as test_inputs/outputs in build_model.

this turned out to be wrong... in the deepimagej they are treated differently... (this should be fixed now though

needed to resolve relative paths
@FynnBe
Copy link
Member

FynnBe commented Nov 27, 2021

An update to bioimageio.spec should fix the remaining issue of serialization: bioimage-io/spec-bioimage-io#294

@constantinpape
Copy link
Contributor Author

Thanks @FynnBe now it all works.

@constantinpape constantinpape merged commit f27b40a into main Nov 28, 2021
@constantinpape constantinpape deleted the deepimagej-config branch November 28, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants