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

checker falsely flags halo inside shape #56

Closed
k-dominik opened this issue Apr 26, 2021 · 10 comments
Closed

checker falsely flags halo inside shape #56

k-dominik opened this issue Apr 26, 2021 · 10 comments

Comments

@k-dominik
Copy link
Member

k-dominik commented Apr 26, 2021

see bioimage-io/spec-bioimage-io#76

best guess is that the problem might be here: https://github.com/bioimage-io/python-bioimage-io/blob/2f7184f0f35e8dd195c4045247fdefa9fabc5502/pybio/spec/schema.py#L91-L95

@constantinpape
Copy link
Contributor

Yes, it looks like halo was mistakenly put here
instead of the OutputShape.
cc @FynnBe

@FynnBe
Copy link
Member

FynnBe commented Apr 26, 2021

I think the error lies in bioimage-io/spec-bioimage-io#76

In our "reference example" halo is also part of the output tensor description, not of the shape:
https://github.com/bioimage-io/pytorch-bioimage-io/blob/113e6d01989e39cd955cb2ead311086548d95026/specs/models/unet2d_nuclei_broad/UNet2DNucleiBroad.model.yaml#L45

This is because the shape might be defined with reference_input, etc. but it may also be a fixed shape : [1, 2, 3], and halo may be specified independently...
I see some arguments for including halo under shape, but as of now that is indeed 'wrong'

@constantinpape
Copy link
Contributor

I think the error lies in bioimage-io/spec-bioimage-io#76

In our "reference example" halo is also part of the output tensor description, not of the shape:
https://github.com/bioimage-io/pytorch-bioimage-io/blob/113e6d01989e39cd955cb2ead311086548d95026/specs/models/unet2d_nuclei_broad/UNet2DNucleiBroad.model.yaml#L45

I am pretty sure that we have put halo in the shape (going back to the discussions about how to compute the output shape).
This is rather an error here and in the reference example.

@constantinpape
Copy link
Contributor

This also becomes clear if you actually read what we have in the spec description:

- reference_input name of the reference input tensor
- scale list of factors 'output_pix/input_pix' for each dimension
- offset position of origin wrt to input
- halo the halo to crop from the output tensor (for example to crop away boundary efects or for tiling). The halo should be croped from both sides, i.e. shape_after_crop = shape - 2 * halo. The halo is not cropped by the bioimage.io model, but is left to be cropped by the consumer software. Use offset if the model output itself is cropped.

This refers to offset without any further modifiers, which strongly implies that its on the same level of hierarchy.

@FynnBe
Copy link
Member

FynnBe commented Apr 26, 2021

I see how this ambiguity arose over time, as we had further discussions about the meaning of halo and offset. While offset only makes sense with the presence of reference_input, halo is independent of how the output shape is defined. If at any time we decided to move halo into shape I am not opposed to changing the validator accordingly, but I am puzzled about the use case of a fixed output shape that has a halo. Because of this case I see no other way (without further changes) to keep halo out of shape.

@constantinpape
Copy link
Contributor

If at any time we decided to move halo into shape

It was always part of shape, see these commits were we introduced the shape descriptions:
bioimage-io/spec-bioimage-io@9d1dfb6
bioimage-io/spec-bioimage-io@dc54550

I am puzzled about the use case of a fixed output shape that has a halo

This is again going back to the discussions we had about this, but halo is a hint for the consumer software on what parts of the output to crop away in order to get the "valid" part of the prediction, e.g. to avoid border effects.
So in that sense it should not matter if the output shape is fixed or computed from offset and scale, the treatment of halo is always identical.

@FynnBe
Copy link
Member

FynnBe commented Apr 26, 2021

So in that sense it should not matter if the output shape is fixed or computed from offset and scale, the treatment of halo is always identical.

exactly, but in case shape is specified explicitly, halo cannot be below shape like offset and scale

I am thinking of this scenario:

output:
  shape:  [10, 20, 30]  # explicit shape
  halo: [1, 2, 3]  # halo is outside of shape

@constantinpape
Copy link
Contributor

constantinpape commented Apr 26, 2021

Ok, I agree. This is an error in the spec (but note that it was an error from the beginning....). I will add a PR to fix it in there properly.
I think the fundamental problem is that we allow too many different ways of specifying the shape, which change the type of the shape field. This is not good design, but it's probably not worth reopening this can of worms now.

@constantinpape
Copy link
Contributor

See bioimage-io/spec-bioimage-io#77.

@constantinpape
Copy link
Contributor

This is fixed.

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

3 participants