Skip to content

Conversation

@f-schnabel
Copy link
Contributor

@f-schnabel f-schnabel 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.

Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
@f-schnabel
Copy link
Contributor Author

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

@f-schnabel f-schnabel marked this pull request as ready for review December 17, 2022 00:14
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
@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))

@f-schnabel
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

@f-schnabel
Copy link
Contributor Author

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

Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
@f-schnabel f-schnabel force-pushed the feature_deprecated_default branch from a55a8a0 to 41c9b5f Compare December 21, 2022 12:20
@f-schnabel
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
@f-schnabel f-schnabel 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