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

regionprops: consistent naming scheme #5213

Closed
maxfrei750 opened this issue Feb 1, 2021 · 10 comments · Fixed by #5254
Closed

regionprops: consistent naming scheme #5213

maxfrei750 opened this issue Feb 1, 2021 · 10 comments · Fixed by #5254
Labels
📜 type: API Involves API change(s)

Comments

@maxfrei750
Copy link
Contributor

Description

It would be nice to have a consistent naming scheme for the properties retrieved by the measure.regionprops function.

For instance, there are feret_diameter_max and max_intensity. It would be nice to have intensity_max instead.

Other candidates are:

mean_intensity
min_intensity
bbox_area
convex_area
convex_image
filled_area
filled_image
intensity_image
major_axis_length
max_intensity
mean_intensity
min_intensity
minor_axis_length
weighted_centroid
weighted_local_centroid
weighted_moments
weighted_moments_central
weighted_moments_hu
weighted_moments_normalized

Since these changes would not be backwards compatible, they could be targeted for version 1.0.

Of course, it would also be a possibility, to rename feret_diameter_max to max_feret_diameter. However, this would prevent similar properties (i.e. all the different kinds of areas, intensities, moments and images) to be grouped together based on their name (see the discussion in #4820).

@maxfrei750
Copy link
Contributor Author

If this change is welcome, then I could create a PR for it.

@jni
Copy link
Member

jni commented Feb 3, 2021

Thanks @maxfrei750! Certainly I'd like to revisit these and I agree that for 1.0 we want to make sure we like all of the naming conventions for regionprops — one of our most important functions. We should probably wait for a bit of discussion here, from everyone and especially from @scikit-image/core, about what names they would consider changing from regionprops.

@stefanv
Copy link
Member

stefanv commented Feb 4, 2021

I suggest @maxfrei750 comes up with a proposal for a naming scheme, and we leave the old names in place while updating the documentation to only provide the new ones. Then, set a TODO to update to only the new names in a couple of releases (nothing urgent).

@rfezzani rfezzani added the 📜 type: API Involves API change(s) label Feb 5, 2021
@maxfrei750
Copy link
Contributor Author

As requested by @stefanv here is a proposal for a naming scheme:

Naming Scheme

{generic property}_{1st most frequent specification}_{2nd most frequent specification}_ ... _{nth most frequent specification}

generic property

  • properties that exist multiple times
  • specifies type and shape of value that will be returned
  • examples: image, intensity, moments, etc.

1st most frequent specification

  • specification of a generic property that occurs most frequently
  • example: weighted

2nd most frequent specification

  • specification of a generic property that occurs second-most frequently
  • examples: hu, normalized

...

Problems

  1. What is the generic property? For instance major_axis_length: Should it be axis_length_major or length_axis_major? If we go with axis_length_major, should we change it to axislength_major, to emphasize what's the generic property, and what's the specification?
  2. The frequency of specifications may change over time and would then require an API breaking change. However, IMHO, this is rather unlikely to happen anytime soon.
  3. inertia_tensor and inertia_tensor_eigvals might require an exception from the naming scheme. Following the scheme, they should be named inertia_tensor and eigvals_inertia_tensor, but then they would not be grouped together in the documentation.

Additional considerations

We could use this opportunity to rename some properties to be a little more explicit:

  • bbox => bounding_box
  • eigvals => eigenvalues

Examples

area_bbox <= bbox_area
area_convex <= convex_area
area_filled <= filled_area

image_convex <= convex_image
image_filled <= filled_image
image_intensity <= intensity_image

intensity_max <= max_intensity
intensity_mean <= mean_intensity
intensity_min <= min_intensity

axislength_minor <= minor_axis_length
axislength_major <= major_axis_length

centroid_weighted <= weighted_centroid
centroid_weighted_local <= weighted_local_centroid

moments_weighted <= weighted_moments
moments_weighted_central <= weighted_moments_central
moments_weighted_hu<= weighted_moments_hu
moments_weighted_normalized <= weighted_moments_normalized

@stefanv
Copy link
Member

stefanv commented Feb 11, 2021

@maxfrei750 This looks great, thanks!

The one I don't find particularly intuitive is axislength. Perhaps we should consider returning the vectors axis_major and axis_minor of which the norm is the length currently calculated?

@stefanv
Copy link
Member

stefanv commented Feb 11, 2021

I also think inertia_tensor and inertia_tensor_eigvals do make sense in your new scheme. eigvals is more specific than inertia_tensor.

@maxfrei750
Copy link
Contributor Author

@stefanv Thanks for the feedback.

The one I don't find particularly intuitive is axislength. Perhaps we should consider returning the vectors axis_major and axis_minor of which the norm is the length currently calculated?

That would be quite powerful, because it would also allow the calculation of the tilt angle of the axis.

To not lose any functionality, we could then have:

axis_major
axis_minor

length_axis_major
length_axis_minor

angle_axis_major
angle_axis_minor

Would you advise to couple this change of functionality with the PR that implements the renaming?

I also think inertia_tensor and inertia_tensor_eigvals do make sense in your new scheme. eigvals is more specific than inertia_tensor.

I agree that the naming matches the specificity rule. However, inertia_tensor_eigvals does not satisfy the requirement that the very first part of the name "specifies type and shape of value that will be returned". Nevertheless, personally, I think that's ok.

@stefanv
Copy link
Member

stefanv commented Feb 16, 2021

I have the same intuition about the above: axis is the broadest grouping, then major/minor, then length/angle, so I'd prefer axis_major_length, e.g.

@maxfrei750
Copy link
Contributor Author

Agreed. I'll create a PR, when I get to it.

@maxfrei750
Copy link
Contributor Author

While working on the PR, it occurred to me that for reasons of consistency, it might be good to rename the arguments of regionprops and regionprops_table:

label_image => image_label
intensity_image => image_intensity

Maybe even

extra_properties => properties_extra

@stefanv What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 type: API Involves API change(s)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants