Introduce a consistent naming scheme for measure.regionprops

[maxfrei750]

Description

Closes #5213.

For reviewers

• Check that the PR title is short, concise, and will make sense 1 year
  later.
• Check that new functions are imported in corresponding __init__.py.
• Check that new features, API changes, and deprecations are mentioned in
  doc/release/release_dev.rst.

[emmanuelle]

@maxfrei750 thanks for the PR! Is the PR a proof of concept that should be merged for 1.0 or do you intend to have it merged ASAP? In the latter case we should keep the old names as well for compatibility (and just return the new prop of course).

[maxfrei750]

@emmanuelle In #5213 @jni proposed to merge this in version 1.0, so I'd keep it that way, if you agree.

[maxfrei750]

@emmanuelle Does this PR need any changes to be merged in 1.0?

[grlee77]

The code here looks good to me and I think the naming scheme is better than before. I think the question is whether we want to provide a transition period in 0.19 where the old names still work, but warn users that the names will change in 1.0. @scikit-image/core what do you think?

If we do just switch the names in 1.0, we should add a summary table somewhere in the release notes to help users with the transition. Most likely we can write a short guide to the main API changes in 1.0 outside of this PR.

[grlee77]

This was a related naming change discussed in #4821. While we are already changing things here, wouldn't it make sense to do this at the same time?

[maxfrei750]

@grlee77 Good point. If the renaming proposed in #4821 is wanted, then I can add this here. So the question is: Is it wanted?

[VolkerH]

If we do just switch the names in 1.0, we should add a summary table somewhere in the release notes to help users with the transition. Most likely we can write a short guide to the main API changes in 1.0 outside of this PR.

Maybe a utililty function that can translate old names to new names would be useful for transitioning old code.

[stefanv]

If we do just switch the names in 1.0, we should add a summary table somewhere in the release notes to help users with the transition. Most likely we can write a short guide to the main API changes in 1.0 outside of this PR.

Maybe a utililty function that can translate old names to new names would be useful for transitioning old code.

It may be easiest to just leave the old names in place as well, but hide them (or not advertise them).

[grlee77]

Hi @maxfrei750, the naming scheme used here looks great and I want to try and help get this in for a 0.19 release (assuming we are going to also maintain the older snake case names in an undocumented fashion (as @stefanv suggested in the prior comment))

Actually, looking at this more closely, I think the current PROPS dictionary's purpose was to maintain compatibility with an even older set of CamelCase names used in scikit-image<=0.8. So, we should not change any keys in that PROPS dictionary or we will break that backwards compatibility. I will make a PR against your branch with what I think this needs to look like to keep everything happy for old code across versions!

[grlee77]

@maxfrei750: Please see maxfrei750#1 which updates the PROPS dict in the suggested fashion and fixes a test failure that may have been introduced when I merged main here. (If you merge that PR in your fork the commits will automatically show up here).

[maxfrei750]

@grlee77 Thanks for your contribution and especially for catching my mistake with the PROPS dictionary. That definitively could have caused some mayhem for people using older versions. 😅

[grlee77]

I see that #4821 also suggested equivalent_diameter -> equivalent_diameter_area. If we are going to make that change (seems reasonable to me), then let's do it here as well.

[maxfrei750]

@grlee77 Good point! Thanks for pointing that out. Feel free to incorporate this change, or I will do it myself, as soon as I find the time.

[grlee77]

@grlee77 Good point! Thanks for pointing that out. Feel free to incorporate this change, or I will do it myself, as soon as I find the time.

I will go ahead and add it now

[maxfrei750]

@grlee77 Thanks!

[grlee77]

Note that when recently discussing various API-related things, there was a point brought up that "max intensity", etc. read as more clearly than "intensity max", etc. While this is true, I don't personally think that outweighs the benefit of the more coherent organization proposed here (see also discussion in #5213)

@scikit-image/core, please speak out in the next week or so if there are still any strong objections to making this change. If we are going to do it, let's get it in for 0.19.

We had also discussed whether or not to drop the undocumented legacy property names at some point, but that is more suited to a separate PR if we decide to go that route.

[stefanv]

I agree that the more logical/hierarchical organization is worth the slight reduction in "intuitive" readability here. I.e., intensity_max better than max_intensity.

I am -0 on removing the legacy names—not a big deal.

[mkcor]

Hi @grlee77,

Thanks for the reminder through #5169 (comment). I don't have strong objections either way. So I'm happy to follow @stefanv on this one.

[mkcor]

Since this is an internal function, I guess the renaming wasn't necessary... Fine, though.

[mkcor]

For example, function get_intensity_image and attribute _intensity_image are not being renamed here...