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 property getters for all deprecated regionprops names #6000

Merged

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Nov 2, 2021

Description

This is a backwards compatibility PR addressing the following issue raised on image.sc: https://forum.image.sc/t/scikit-image-regionprops-minor-axis-length-versus-axis-minor-length/59223/4. Prior to this PR, the old names were accessable via region['old_name'], but not via an attribute like region.old_name. After this PR, both forms of access will work.

I think we can drop all these old names for skimage2, but it is probably best to keep them working for now. To discourage use, these outdated names were not added to the regionprops function docstring and the autogenerated property docstrings have a line stating the property was deprecated.

I created a mixin class with the deprecated property methods by pasting the text output of the following script into _regionprops.py.

from skimage.measure._regionprops import PROPS

deprecated_props_class = "class DeprecatedProperties:\n"
for k, v in PROPS.items():

    deprecated_props_class += f"""
    @property
    def {k}(self):
        return self.{v}
    """
print(deprecated_props_class)

I thought about also adding a UserWarning to each outdated property, but was not sure it wouldn't be more annoying than useful.

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

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.

This is done for backwards compatibility.
@grlee77 grlee77 added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Nov 2, 2021
if value is not None:
return value
else: # backwards compatibility
return getattr(self, PROPS[key])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this if/else case since all old properties now have attributes via the DeprecatedProperties mixin class.

Copy link
Member

Choose a reason for hiding this comment

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

@grlee77 perhaps we can keep this and just add the old property names to the dict?

Copy link
Member

Choose a reason for hiding this comment

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

Oh right but that doesn't help with attribute access. Perhaps we can add a __getattr__ to the class?

@grlee77 grlee77 modified the milestones: 0.18, 0.19 Nov 4, 2021
grlee77 and others added 3 commits November 7, 2021 19:29
these have been missing for multiple releases without complaint so no need to
pollute the API with them now
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@grlee77
Copy link
Contributor Author

grlee77 commented Nov 8, 2021

whoops. let me fix the conflicts here...

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 8, 2021

This PR was updated to remove properties for the CamelCase names per discussion with @jni. Those have not been there for multiple releases without complaint, so let's not add them back in now...

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

@grlee77 I'm approving, but what do you think of my suggestion of using a class getattr instead? I don't love that the old names now live in a dictionary and a class at the same time...

add __setattr__ as well to disallow setting deprecated properties as well
@pep8speaks
Copy link

pep8speaks commented Nov 8, 2021

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-08 23:41:59 UTC

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 8, 2021

@grlee77 I'm approving, but what do you think of my suggestion of using a class getattr instead? I don't love that the old names now live in a dictionary and a class at the same time...

There is already a __getattr__ to deal with extra user-provided properties, so we can just modify that. I just pushed a commit with this. If you don't like it we can revert it.

To prevent the user from being able to do assignments to the legacy property names (e.g. calling something like stats.major_axis_length = 5) I also implemented __setattr__ which will end up raising an AttributeError as it does for an @property without a setter. Also, this way also does not provide tab completion for the deprecated names when working interactively in an IPython terminal. I would argue that lack of tab-completion is probably a feature in this case, though, as we don't want new users discovering these deprecated property names!

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

@grlee77 I like this better! I've just suggested reverting a couple of strange formatting choices. I'll try pushing them up to your branch.

skimage/measure/_regionprops.py Outdated Show resolved Hide resolved
skimage/measure/_regionprops.py Outdated Show resolved Hide resolved
skimage/measure/_regionprops.py Outdated Show resolved Hide resolved
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants