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

Size of structuring elements a bit inconsistent #4536

Open
sciunto opened this issue Mar 30, 2020 · 32 comments
Open

Size of structuring elements a bit inconsistent #4536

sciunto opened this issue Mar 30, 2020 · 32 comments

Comments

@sciunto
Copy link
Member

sciunto commented Mar 30, 2020

Description

looking at the figure in #4528, it let me realize that the size of the structuring elements are a bit inconsistent, a fact I was not particularly aware of.

My use case is the following: while doing some processing, we might want to change the size and the shape of the structuring element. For instance, changing from square to circle, the user must be cautious to apply a factor two to get the same typical size.

Personally, I find this annoying.

@sciunto sciunto added this to the 1.0 milestone Mar 30, 2020
@rfezzani
Copy link
Member

I agree @sciunto, the inconsistency is even more visible looking at the API:

  • ball(radius)
  • cube(width)
  • diamond(radius)
  • disk(radius)
  • octagon(m, n)
  • octahedron(radius)
  • rectangle(width, height)
  • square(width)
  • star(a)

I would harmonize the API using the radius argument name, since structuring elements are usually required to have even shapes...

@jni
Copy link
Member

jni commented Mar 30, 2020

Hi all!

I don't think square(radius=5) makes much sense. People think of squares/rectangles as having a side length, and circular objects having a radius. How would one manage a rectangle with a radius anyway?

Things like radius vs diameter are another story... We should pick one and stick to it. Given the side length argument for cubes, perhaps diameter is a better option. But, that is a lot of API churn for not a lot of gain. Perhaps something for the 1.0 transition? ;)

@rfezzani
Copy link
Member

radius argument name must refer to the radius of the bounding box that encompass the desired structuring element. Using this definition, square(radius=5) doesn't bother me that much :-).

Anisotropic shapes such as rectangle and octagon are however more touchy even if radii=(2, 3) is possible :-p

This issue can be linked to #4154 and #4464 will help to deal with the API churn ;-)

@jni
Copy link
Member

jni commented Mar 30, 2020

@rfezzani

Anisotropic shapes such as rectangle and octagon are however more touchy even if radii=(2, 3) is possible :-p

Your face says it all. =P

My personal preference is to use width/side length/diameter as the parameter. With radius, I am never sure if the centre pixel is included — does radius 1 correspond to a diameter of 3 or 2? We know 3 because we know it should be odd, but from a user perspective it's quite confusing really.

@sciunto
Copy link
Member Author

sciunto commented Mar 31, 2020

My suggestion is to pass a size parameter for all, corresponding to the bbox:

  • circle(size) -> circle(3) [diameter]
  • rectangle(size) -> rectangle((3, 5)) [side length]

@rfezzani
Copy link
Member

rfezzani commented Mar 31, 2020

@jni

We know 3 because we know it should be odd, but from a user perspective it's quite confusing really.

Isn't education one of the goals of skimage?

I personally didn't know what happens when selem shape is even, I had to dive in scipy code (which is used internally by skimage) to know what happens in this case: selem's center is shifted left... Not really obvious/intuitive...

@jni
Copy link
Member

jni commented Mar 31, 2020

Isn't education one of the goals of skimage?

yep! We can educate by saying "diameter must be an odd number" in every docstring. =)

@rfezzani
Copy link
Member

rfezzani commented Mar 31, 2020

Well, radius makes the resulting shape odd by design... It is already used in four functions (ball, disk, diamond and octahedron) and see no user complaining or confused by its use...

yep! We can educate by saying "diameter must be an odd number" in every docstring. =)

Should we then check if diameter is odd at every function call? Should we warn or raise an error? In fact generating even shaped diamonds and disks will not be obvious...

What about considering "anisotropic" selem (rectangle and octagon) particular cases?

@rfezzani
Copy link
Member

rfezzani commented Apr 1, 2020

@jni and @sciunto, I just checked and found that octagon produces square shaped selem. rectangle is then the only function that outputs non square structuring elements.

We may then reasonably consider rectangle as a particular case.

Now, decision have to be taken regarding the name of the parameter controlling the shape of the remaining selem functions to fix this issue and provide a consistent behavior for selem module's functions. We have two options:

  • size/diameter/width/side_length that describes the side length of the selem bbox with the necessity of taking care of even values (raising error?)
  • radius with the confusion that it may introduce in users mind concerning the output size (central pixel included or not)

As you already noticed, may preference goes to the second option :-). May be other @scikit-image/core members can take part to the decision...

@jni
Copy link
Member

jni commented Apr 1, 2020

@rfezzani I found another argument against radius 😜: is the radius of a square the distance from the centre to the side, or to the corner?

@sciunto
Copy link
Member Author

sciunto commented Apr 1, 2020

That's why I prefer a more generic word, for which the precise definition is provided in each docstring.

@rfezzani
Copy link
Member

rfezzani commented Apr 1, 2020

@rfezzani I found another argument against radius stuck_out_tongue_winking_eye: is the radius of a square the distance from the centre to the side, or to the corner?

@jni, seriously? Did I ever said that a square has a radius, (even if it is sometimes True: the 2D ball with L_inf norm is a square)? I really don't understand why this hurts you that much...

@sciunto, size is fine for me, just be aware that using this convention forces us to check for odd values, which I find unnecessary/inelegant...

@rfezzani
Copy link
Member

rfezzani commented Apr 1, 2020

An other argument in favor of radius, it will introduce less deprecation...

@alexdesiqueira
Copy link
Member

Hey everyone,
if we are actually thinking on doing this, IMHO we need to set an acceptable parameter — here size would be a way better choice than radius, for instance. Size is generic enough for us to define what size is in all docstrings.

@alexdesiqueira
Copy link
Member

@jni, seriously? Did I ever said that a square has a radius, (even if it is sometimes True: the 2D ball with L_inf norm is a square)? I really don't understand why this hurts you that much...

Please keep it friendly @rfezzani. We are all brothers and sisters trying to reach an optimal solution here 😊

@rfezzani
Copy link
Member

rfezzani commented Apr 1, 2020

@alexdesiqueira / @jni, sorry if you felt it not friendly, it was absolutely not my intention, please put on account of my poor english...

@rfezzani
Copy link
Member

rfezzani commented Apr 1, 2020

Btw, irony is not what I call super friendly either ;-)

@jni
Copy link
Member

jni commented Apr 2, 2020

@rfezzani

Did I ever said that a square has a radius,

You said this:

Using this definition, square(radius=5) doesn't bother me that much :-).

which was close enough. =)

Absolutely agree with everything else that was said — there's no enmity, I absolutely love the work and proposals coming up here, we just need to find a consensus, and preferably have a good time doing it! =D Different people have different preferences, and even in objective cases, reasonable people can come to different conclusions. Eventually, we'll have to make a decision, but there is no urgency. Let's just have people voice their preferences and arguments and eventually we can decide on the least offensive option. =)

Semi-exhaustive list of options:

  • leave things as is more or less
  • normalise on radius or similar
  • normalise on side_length/size or similar
  • rectangle-like things have size/side_length and circle-like things have radius/diameter, but make those two categories consistent with each other
  • normalise on a more generic term than radius, say, "half_length" or "half_side_length"

@rfezzani
Copy link
Member

rfezzani commented Apr 2, 2020

Thank you @jni for this good summary.

  • leave things as is more or less

I am not in favor of this option, since it leaves this issue unfixed...

  • normalise on radius or similar

Apparently not accepted...The API becomes

  • ball(radius)
  • cube(radius) -> Deprecation
  • diamond(radius)
  • disk(radius)
  • octagon(radius, side_length=None) -> Deprecation
  • octahedron(radius)
  • rectangle(radius, height=None) -> Deprecation
  • square(radius) -> Deprecation
  • star(radius) -> Deprecation
  • normalise on side_length/size or similar

I am fine with this option, but IMHO we should not support even values.The API becomes

with side_length/size

  • ball(size) -> Deprecation + input validation
  • cube(size) -> Deprecation + input validation
  • diamond(size) -> Deprecation + input validation
  • disk(size) -> Deprecation + input validation
  • octagon(size, side_length=None) -> Deprecation + input validation
  • octahedron(size) -> Deprecation + input validation
  • rectangle(size) -> Deprecation + input validation
  • square(size) -> Deprecation + input validation
  • star(size) -> Deprecation + input validation

with width

  • ball(width) -> Deprecation + input validation
  • cube(width) -> input validation
  • diamond(width) -> Deprecation + input validation
  • disk(width) -> Deprecation + input validation
  • octagon(width, side_length=None) -> Deprecation + input validation
  • octahedron(width) -> Deprecation + input validation
  • rectangle(width, height=None) -> input validation
  • square(width) -> input validation
  • star(width) -> Deprecation + input validation
  • rectangle-like things have size/side_length and circle-like things have radius/diameter, but make those two categories consistent with each other

This option doesn't address the inconsistency pointed in this issue

  • normalise on a more generic term than radius, say, "half_length" or "half_side_length"
  • ball(half_length) -> Deprecation
  • cube(half_length) -> Deprecation
  • diamond(half_length) -> Deprecation
  • disk(half_length) -> Deprecation
  • octagon(half_length, side_length=None) -> Deprecation
  • octahedron(half_length) -> Deprecation
  • rectangle(half_length, height=None) -> Deprecation
  • square(half_length) -> Deprecation
  • star(half_length) -> Deprecation

@rfezzani
Copy link
Member

rfezzani commented Apr 2, 2020

I think that it is necessary to rethink star and octagon argument names (respectively a and (m, n)): you may be agree that it is not really descriptive...

@sciunto
Copy link
Member Author

sciunto commented Apr 2, 2020

I'm fine with both size and half_size (and half_somethingelse). Just to say also that in threshold_local, the block size must be odd and a check is performed. I don't say it is ever good or bad, it's just to feed the debate.

@alexdesiqueira
Copy link
Member

I'm not happy with half_size; I guess size is generic enough to be described according to the object.

@stefanv
Copy link
Member

stefanv commented Apr 2, 2020

I missed out on all the fun :) Why are we providing rectangle and square in the first place? Can't those trivially be created using ones? The others seem not to be controversial, so throwing those out gives us a consistent interface. In terms of wording: radius implies the thing fits into a sphere. That's not always the case for selems.

So, can we use size, and always generate square selems? Non-square selems seem like odd constructs anyway.

@stefanv
Copy link
Member

stefanv commented Apr 2, 2020

(We may have to enforce size odd in most cases.)

@alexdesiqueira
Copy link
Member

So, can we use size, and always generate square selems? Non-square selems seem like odd constructs anyway.

Non-square selems are necessary for lots of applications (e.g., segmentation of cells-like elements). I'd be sad if they cease to exist ☺️

@stefanv
Copy link
Member

stefanv commented Apr 2, 2020

So, can we use size, and always generate square selems? Non-square selems seem like odd constructs anyway.

Non-square selems are necessary for lots of applications (e.g., segmentation of cells-like elements). I'd be sad if they cease to exist relaxed

I did not say we should not support them; the question is more whether we provide pre-packaged non-square selems.

@rfezzani
Copy link
Member

rfezzani commented Apr 2, 2020

Even if rectangle is with no doubt a useful selem, I think that it can be replaced without much hurt for the user by np.ones. I am +1 for the suggestion of @stefanv of deprecating cube square and rectangle in favor of np.ones...

@alexdesiqueira
Copy link
Member

alexdesiqueira commented Apr 2, 2020

I did not say we should not support them;

I didn't consider that as well. 😅

the question is more whether we provide pre-packaged non-square selems.

I think they are useful and should continue where they are. But hey, my two cents only.

@rfezzani
Copy link
Member

rfezzani commented Apr 2, 2020

radius implies the thing fits into a sphere. That's not always the case for selems.

Damn, only octagon doesn't always fit in a circle... 😄

@rfezzani
Copy link
Member

rfezzani commented Apr 5, 2020

The problem with the skimage current use of even selem shapes is that the behavior is not explicit and that the user have no control on it!
I think that using the origin argument (as it is done in scipy.ndimage.morphology) addresses this problem.
Even selem shapes are no more an issue at selem creation but at selem use. I mean that an error or a warning is only necessary if the selem shape is even and origin is 0...

The use of size as generic argument has then no drawback apart the necessity for deprecation.

@sciunto
Copy link
Member Author

sciunto commented Jun 3, 2021

Since there is deprecation going on for 0.20, perhaps we must consider this?

@rfezzani
Copy link
Member

rfezzani commented Jun 7, 2021

Thank you @sciunto. Is the proposed solution above (introducing the origin argument and using the size generic argument) OK for you?

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

No branches or pull requests

6 participants