Change default of inject noise site to "after"#372
Conversation
ihincks
left a comment
ihincks
left a comment
There was a problem hiding this comment.
Thanks Josh!
While it is in our rights to make this breaking change, I'd suggest doing it via a deprecation period in this case. So, assign the default values to None for now, have None be coerced into "before" so that this PR doesn't introduce a change, but also have None (but not "before") cause a deprecation warning. Then, in the next release or the one after, we can make the break.
ihincks
left a comment
ihincks
left a comment
There was a problem hiding this comment.
Should we treat the InjectNoise annotation in the same way as generate_boxing_pass_manager?
| if inject_noise_site is None: | ||
| warnings.warn( | ||
| "The default of the 'inject_noise_site' argument will be changed from " | ||
| "`'before'` to after in version 0.21.0.", |
There was a problem hiding this comment.
| "`'before'` to after in version 0.21.0.", | |
| "`'before'` to `"after"` no sooner than version 0.21.0.", |
| warnings.warn( | ||
| "The default of the 'inject_noise_site' argument will be changed from " | ||
| "`'before'` to after in version 0.21.0.", | ||
| DeprecationWarning, |
There was a problem hiding this comment.
I wonder if we want a FutureWarning here instead of a DeprecationWarning: the target audience is users and not CI, afaik.
https://docs.python.org/3/library/warnings.html#warning-categories
2 participants
Summary
Changes the defaults of the boxing pass manager and the AddInjectNoise pass to use "after" as the default.
Details and comments