MNT: Set stacklevel manually for warnings. by hmaarrfk · Pull Request #3467 · scikit-image/scikit-image

hmaarrfk · 2018-10-10T01:40:40Z

Add a test to catch future unset stacklevels.
The test added here needs this PR to pass: #3459

@lagru your help on this would be appreciated.

Either this, or #3438 should be pulled in.

Depends on what style you like. See discussion on #3438 for more details.

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

Clean style in the spirit of PEP8
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

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

For reviewers

(Don't remove the checklist below.)

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.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

lagru · 2018-10-13T08:49:41Z

@lagru your help on this would be appreciated.

Of course. I'll have a look once I have access to my notebook again.

hmaarrfk · 2018-10-13T16:05:55Z

I’m not so sure if this te correct way to go about it. This is a pretty big change to many different files. I don’t think warnings are as rigorously tested as other aspects of the code. I think the previous fun tools.partial solution is leas intrusive and we can “discourage” the use of that convenience if we want to.

hmaarrfk · 2018-11-03T15:31:09Z

@stefanv I know this change is much more intrusive, but it allows the use of that scipy script you mentioned.

I think it is a much better fix long term.

Definitely in the right direction for usability.

hmaarrfk · 2018-11-16T10:40:04Z

@scikit-image/core your input on how to deal with the code vendored from scipy would be helpful.

I can rebase once #3458 gets in.

soupault

2 comments, otherwise 👍. Thanks Mark!

soupault · 2019-03-30T12:40:02Z

skimage/viewer/__init__.py

 if not has_qt:
-    warn('Viewer requires Qt')
+    from warnings import warn
+    warn('Viewer requires Qt', stacklevel=2)


Should be 1?

Yea, could be 1. I tested this quite a bit, doesn't actually matter on what you select.

soupault · 2019-03-30T12:44:55Z

skimage/feature/register_translation.py

                             "data's number of dimensions.")

-    im2pi = 1j * 2 * np.pi
+    # For the 2D case, it is more efficient to use the well-optimized


Is this change intended to be here?

ping @hmaarrfk something weird in your history...

Yeah. I'm really not sure. I didn't have time to figure out why the rebase went astray

Thahnks you two. I think what happened was that that function was simplified. I was more careful with my rebase now and I think all is well.

jni · 2019-04-03T02:03:34Z

@hmaarrfk thanks for the update! You've missed a couple of the old-style imports it seems:

https://dev.azure.com/scikit-image/scikit-image/_build/results?buildId=231

hmaarrfk · 2019-04-03T12:38:27Z

I guess a few more warns where added. I hope I got the stack levels right...

hmaarrfk · 2019-05-10T02:59:43Z

@scikit-image/core I just rebased this for 0.16. If anybody wanted to look at this, I think it will help users follow our changes in their code and help them transition as we add/improve the library. thanks

stefanv · 2019-05-10T05:44:40Z

skimage/_shared/tests/test_warnings.py

+
+
+"""
+Copyright (c) 2001, 2002 Enthought, Inc.


We distribute all license text in the main LICENSE.txt now.

Do you want to remove it from this file as well? I find it useful to keep the license in the file for these single "borrowed" files from libraries.

Either way is fine, as long as it is listed in the main license file as well.

stefanv · 2019-05-10T05:48:11Z

Would it make sense to have a default stacklevel of 2, rather than specifying it almost everywhere.

Also, can we provide some guidance to developers on how to determine the correct stacklevel?

hmaarrfk · 2019-05-10T11:14:17Z

I think i had a PR where the default stack level was simply set to 2, unfortunately, when refactoring code, it becomes common to put warnings in a common function where the stack level should be 3. In that case, incrementing the stacklevel often gets forgotten.

Warnings, as shown by many of Issues recently, warnings are quite intrusive (though not as intrusive as your code breaking) and thus I think they warrant some thought at the time the PR is made. I think the few times where stack level is set to 3 makes it quite important to ask the contributor to think about who should receive the warning.

Generally, you should set them so that users get pointed to their line of code, where they can take an action to remedy the situation.

hmaarrfk · 2019-05-10T11:22:48Z

I think i had a PR where the default stack level was simply set to 2, unfortunately, when refactoring code, it becomes common to put warnings in a common function where the stack level should be 3.

Warnings, as shown by many of Issues recently, are quite intrusive (though not as intrusive as your code breaking) and thus I think they warrant some thought at the time the PR is made.

Finally, that script you found @stefanv is pretty godly, it parses the python AST to find where warn is called without a stack level. Not sure we can do that with functools partial.

Generally, you should set them so that users get pointed to their line of code, where the user can take an action to remedy the situation.

jni · 2019-05-11T01:10:33Z

I would very much appreciate a paragraph or two about how to correctly use stacklevels in the developer guide. I'm still fuzzy about how to correctly think about them.

Yes, I remember your old PR and people complaining that maybe it wasn't the right thing always. 😂 can't please everyone! fwiw I think I like this better because contributors are more likely to be familiar with warnings.warn than skimage.util.warn.

hmaarrfk · 2019-07-28T23:53:45Z

@scikit-image/core what is the best way to get this through? should we just touchup the internal warn function so that it does what it is intended to do and call it a day? ortherwise, this PR, with all the suggestion (automated testing, documentation) is very hard to keep updated, especially as many other more important PRs need our attention.

That said, a recent PR that added many deprecation warnings basically points users to some obscure location in internal scikit-image functions with no recourse on where in their code they need to make the modifications. I really think this PR adds alot of usability to the deprecation policy.

jni · 2019-07-29T02:06:50Z

@hmaarrfk I still think I like this approach, sorry that it's been hard to get through. When I need to warn I typically git grep warn to see how it's done elsewhere in the library. Combined with my distaste for "just for skimage" approaches, my preference would be to see warnings.warn being used with the correct stacklevel. (#4062 notwithstanding, that can also work for people.)

… unset stacklevels.

LICENSE.txt

hmaarrfk · 2019-07-31T01:32:24Z

The only thing left in this PR is the test that goes through the AST and checks all warnings calls.

Clearly There isn't a concensus on using the internal warn function or the the standard python one.

I opened PRs where the only thing I did was correct the places in here where the warning stack level wasn't set up correctly.

I'm going to close this PR and others are free to reopen if consensus is reached.

hmaarrfk force-pushed the remove_interal_warn_function branch from 6f5d1c3 to 10f8206 Compare November 3, 2018 13:08

This comment has been minimized.

Sign in to view

hmaarrfk force-pushed the remove_interal_warn_function branch 3 times, most recently from f495f41 to 005a175 Compare November 3, 2018 14:47

lagru mentioned this pull request Nov 11, 2018

Fix morphology.local_maxima for input with any dimension < 3 #3447

Merged

9 tasks

sciunto added the 🔧 type: Maintenance Refactoring and maintenance of internals label Nov 11, 2018

sciunto requested a review from stefanv November 11, 2018 19:57

lagru mentioned this pull request Nov 16, 2018

MNT: Set the stack level of _warnings.warn to the correct location #3438

Closed

7 tasks

hmaarrfk force-pushed the remove_interal_warn_function branch from fb601f2 to 02bf9cc Compare February 5, 2019 04:13

hmaarrfk mentioned this pull request Mar 21, 2019

How do I suppress warnings for future deprecation? #3436

Closed

hmaarrfk force-pushed the remove_interal_warn_function branch from 02bf9cc to e0e04fc Compare March 24, 2019 05:09

soupault reviewed Mar 30, 2019

View reviewed changes

hmaarrfk force-pushed the remove_interal_warn_function branch from 576b77f to ed55686 Compare April 3, 2019 01:48

hmaarrfk force-pushed the remove_interal_warn_function branch from b95581b to 11b236c Compare April 3, 2019 12:38

hmaarrfk added this to the 0.16 milestone May 6, 2019

hmaarrfk force-pushed the remove_interal_warn_function branch from 11b236c to 8c2a1bd Compare May 10, 2019 02:58

stefanv reviewed May 10, 2019

View reviewed changes

hmaarrfk force-pushed the remove_interal_warn_function branch from ab80043 to 70ce8c4 Compare July 28, 2019 23:54

hmaarrfk mentioned this pull request Jul 29, 2019

Use functools instead of a real function for the internal warn function #4062

Merged

9 tasks

hmaarrfk added 15 commits July 28, 2019 22:59

MNT: Set stacklevel manually for warnings. Add a test to catch future…

87a1a28

… unset stacklevels.

Add a test to ensure future warnings calls use stacklevels

049dd18

Update a few warn statements and add their stack levels appropriaely

af0adcc

fixup the test to allow filters in conftest.py

8076faa

Add a few missing stack levels

3f381b2

Add missing stacklevels to _median.

bb4c59a

Add a note about stacklevel to the contributing docs.

2332f39

Add a sentence about how to test the stack level

f02a21a

Add the license for skimage/_shared/tests/test_warnings.py in

f2c21e7

LICENSE.txt

Explain what stacklevel does

6b4db44

unused imports.

3de5624

Not sure why filters rank _handle_input should be set to stacklevel=4

792c17a

Simplify documentation

c6f646b

Added scipy's contribution to our license file

0574253

Remove double license for the warnings

adc31e9

hmaarrfk force-pushed the remove_interal_warn_function branch from 70ce8c4 to adc31e9 Compare July 29, 2019 02:59

hmaarrfk closed this Jul 31, 2019

Uh oh!

Conversation

hmaarrfk commented Oct 10, 2018

Checklist

References

For reviewers

Uh oh!

lagru commented Oct 13, 2018

Uh oh!

hmaarrfk commented Oct 13, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

hmaarrfk commented Nov 3, 2018

Uh oh!

hmaarrfk commented Nov 16, 2018

Uh oh!

soupault left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soupault Mar 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jni commented Apr 3, 2019

Uh oh!

hmaarrfk commented Apr 3, 2019

Uh oh!

hmaarrfk commented May 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanv commented May 10, 2019

Uh oh!

hmaarrfk commented May 10, 2019

Uh oh!

hmaarrfk commented May 10, 2019

Uh oh!

jni commented May 11, 2019

Uh oh!

hmaarrfk commented Jul 28, 2019

Uh oh!

jni commented Jul 29, 2019

Uh oh!

hmaarrfk commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

soupault Mar 30, 2019 •

edited

Loading