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

Utility function to warn changes in default arguments #5738

Merged
merged 8 commits into from
Jan 4, 2023

Conversation

Shadow-Devil
Copy link
Contributor

@Shadow-Devil Shadow-Devil commented Dec 14, 2022

Fixes #5653.

Description

A new utility decorator that enables us to warn users of a changing default argument.
Current implementation does the following:

  • If version < since no warning will be printed.
  • If the default argument is explicitly set by the caller no warning will be printed.
  • If since <= version < replaced a warning like this will be printed: "Default of argument {name} has been deprecated since version {since} from {old_default} to {new_default}. It will be changed in version {replaced}."
  • If replaced <= version a warning like this will be printed: "Default of argument {name} was changed in version {changed} from {old_default} to {new_default}."
  • It doesn't validate the old_default, so you can even use this in scenarios where the default is actually None but set later in the function. This also enables us to set old_default to any string if the default is e.g. not printable.
  • The only validation that will throw an error is, if the new_default == the actual default and version < replaced. Which means, that somebody replaced the value already, before the version was incremented. Apart from that also any value for new_default can be set, giving the same advantages as for the old_default.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@Shadow-Devil
Copy link
Contributor Author

I still need to write some tests and probably update the documentation but here is the first draft.

@Shadow-Devil Shadow-Devil marked this pull request as ready for review December 17, 2022 00:14
@wyli
Copy link
Contributor

wyli commented Dec 19, 2022

I looked at this example:

from monai.utils import deprecated_arg_default

@deprecated_arg_default(name="b", old_default="e", new_default="d", since="0.8.0", version_val="1.0.0")
def foo(a, b="e"):
    return a, b

print(foo(1))

this correctly shows that argument b's default has been changed to "d" from "e".

But for the following example, would be great to also provide a forecast for the users, "the default value of argument b will be changed to "d" soon (after version 1.2.0)":

from monai.utils import deprecated_arg_default

@deprecated_arg_default(name="b", old_default="e", new_default="d", since="1.2.0", version_val="1.0.0")
def foo(a, b="e"):
    return a, b

print(foo(1))

@Shadow-Devil
Copy link
Contributor Author

There are two version constraints: since and changed. I think you used since for when the default is changed, but it it means "from which version should we display a warning/this default is deprecated". This was taken from deprecated_args where they are called since and removed. If it's confusing and we only need the version from which it is actually changed then I can adjust it.

@wyli
Copy link
Contributor

wyli commented Dec 20, 2022

ok, changed doesn't exist in the code atm, looks like a typo

@Shadow-Devil
Copy link
Contributor Author

i mean replaced, sorry. I first named the argument "changed" but renamed it to "replaced".

Signed-off-by: Felix Schnabel <[email protected]>
@Shadow-Devil Shadow-Devil force-pushed the feature_deprecated_default branch from a55a8a0 to 41c9b5f Compare December 21, 2022 12:20
@Shadow-Devil
Copy link
Contributor Author

@wyli Do you want me to remove the second parameter? Then we just have since which stands for the version when the default will be/was changed.

@wyli
Copy link
Contributor

wyli commented Jan 4, 2023

/build

Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thank you, this looks good, I'll merge if all tests work fine.

@wyli wyli merged commit 315d2d2 into Project-MONAI:dev Jan 4, 2023
@Shadow-Devil Shadow-Devil deleted the feature_deprecated_default branch January 23, 2023 12:48
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

Successfully merging this pull request may close these issues.

utility function to warn changes in default arguments
2 participants