MNT: Set the stack level of `_warnings.warn` to the correct location by hmaarrfk · Pull Request #3438 · scikit-image/scikit-image

hmaarrfk · 2018-10-02T15:27:35Z

Currently the user experience of warnings is a little all over the place. The warnings are pointing to internal scikit-image functions, see Issue #3436 for example.

I think an attempt to alleviate this was by writing a custom warn function, but it seems that writing it as a function introduced an other stacklevel which was not taken into account in the default parameter choice.

I don't think we should be making our own function for warnings since many contributors might be used to the standard warnings function, and understand what the stacklevel parameter means there.

This fixes this by using functools.partial to avoid the creation of a new stack level. I also modified the stack level in an internal function to 3 to increase the chances of it pointing to the user's code.

I kinda went through searching for import warnings and from warnings and modified the code as needed.

I had some code that would actually test that the warning was provided at the correct location, but I'm not sure how to adapt it for scikit-image's usecase. Depending on the amount of automagic we do, it might still be wrong. For example img_as_[u]int might be called in a rank algorithm at which case the correct warning level would be at least 1 above the standard one.

https://github.com/hmaarrfk/wabisabi/blob/master/wabisabi/tests/test_default_parameter_change.py#L67

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
existing benchmark
Unit tests

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

stefanv · 2018-10-02T22:05:13Z

See also https://github.com/scipy/scipy/blob/master/scipy/_lib/tests/test_warnings.py#L110

hmaarrfk · 2018-10-03T15:11:10Z

@stefanv I have no idea what that test is doing :/ seems very magical to me

stefanv · 2018-10-04T07:08:28Z

It looks to me as if that test verifies that functions use the correct StackLevel.

hmaarrfk · 2018-10-09T01:44:13Z

@stefanv ah i see. I just didn't know what fixtures were. I kinda get it. It parses the whole package and find any reference to warn and tries to check the stack level.

hmaarrfk · 2018-10-09T04:16:08Z

Running that script on the current master (even after a few of my maintenance fixes) yields:

Details

_______________________ test_warning_calls_stacklevels _______________________

warning_calls = ([], ['segmentation/random_walker_segmentation.py:374', 'segmentation/random_walker_segmentation.py:382', 'segmentatio...walker_segmentation.py:473', 'exposure/exposure.py:62', 'morphology/convex_hull.py:52', 'morphology/misc.py:132', ...])

    def test_warning_calls_stacklevels(warning_calls):
        bad_filters, bad_stacklevels = warning_calls
    
        msg = ""
    
        if bad_filters:
            msg += ("warning ignore filter should not be used, instead, use\n"
                    "skimage._shared._warnings.expected_warnings (in tests only);\n"
                    "found in:\n    {}".format("\n    ".join(bad_filters)))
            msg += "\n\n"
    
        if bad_stacklevels:
            msg += "warnings should have an appropriate stacklevel:\n    {}".format(
                    "\n    ".join(bad_stacklevels))
    
        if msg:
>           raise AssertionError(msg)
E           AssertionError: warnings should have an appropriate stacklevel:
E               segmentation/random_walker_segmentation.py:374
E               segmentation/random_walker_segmentation.py:382
E               segmentation/random_walker_segmentation.py:473
E               exposure/exposure.py:62
E               morphology/convex_hull.py:52
E               morphology/misc.py:132
E               morphology/misc.py:207
E               morphology/misc.py:211
E               transform/radon_transform.py:62
E               transform/_warps.py:23
E               transform/_warps.py:129
E               transform/_warps.py:829
E               feature/_hog.py:146
E               feature/_hog.py:246
E               feature/register_translation.py:100
E               feature/corner.py:167
E               feature/corner.py:338
E               external/tifffile/tifffile.py:298
E               external/tifffile/tifffile.py:445
E               external/tifffile/tifffile.py:642
E               external/tifffile/tifffile.py:1089
E               external/tifffile/tifffile.py:1548
E               external/tifffile/tifffile.py:1568
E               external/tifffile/tifffile.py:1631
E               external/tifffile/tifffile.py:1636
E               external/tifffile/tifffile.py:1743
E               external/tifffile/tifffile.py:1753
E               external/tifffile/tifffile.py:1800
E               external/tifffile/tifffile.py:1815
E               external/tifffile/tifffile.py:1826
E               external/tifffile/tifffile.py:1835
E               external/tifffile/tifffile.py:2098
E               external/tifffile/tifffile.py:2111
E               external/tifffile/tifffile.py:2125
E               external/tifffile/tifffile.py:2129
E               external/tifffile/tifffile.py:2315
E               external/tifffile/tifffile.py:2337
E               external/tifffile/tifffile.py:2417
E               external/tifffile/tifffile.py:2547
E               external/tifffile/tifffile.py:2580
E               external/tifffile/tifffile.py:2656
E               external/tifffile/tifffile.py:2695
E               external/tifffile/tifffile.py:2811
E               external/tifffile/tifffile.py:2827
E               external/tifffile/tifffile.py:3045
E               external/tifffile/tifffile.py:3107
E               external/tifffile/tifffile.py:3371
E               external/tifffile/tifffile.py:3675
E               external/tifffile/tifffile.py:3718
E               external/tifffile/tifffile.py:3809
E               external/tifffile/tifffile.py:3839
E               external/tifffile/tifffile.py:3852
E               external/tifffile/tifffile.py:4339
E               external/tifffile/tifffile.py:4353
E               external/tifffile/tifffile.py:4517
E               external/tifffile/tifffile.py:6358
E               io/_io.py:48
E               io/_io.py:53
E               io/_io.py:139
E               io/_io.py:141
E               io/_plugins/matplotlib_plugin.py:72
E               io/_plugins/matplotlib_plugin.py:75
E               io/_plugins/matplotlib_plugin.py:78
E               restoration/unwrap.py:86
E               restoration/_denoise.py:96
E               restoration/_denoise.py:100
E               restoration/_denoise.py:432
E               restoration/_denoise.py:460
E               restoration/_denoise.py:690
E               measure/_structural_similarity.py:148
E               measure/_marching_cubes_classic.py:239
E               measure/simple_metrics.py:127
E               measure/_regionprops.py:250
E               measure/_regionprops.py:260
E               measure/_moments.py:256
E               future/graph/_ncut.py:5
E               future/graph/graph_cut.py:5
E               util/shape.py:93
E               util/shape.py:246
E               util/dtype.py:129
E               util/dtype.py:134
E               util/dtype.py:183
E               draw/_random_shapes.py:349
E               novice/__init__.py:103
E               data/__init__.py:66
E               filters/thresholding.py:270
E               filters/_gaussian.py:122
E               filters/rank/generic.py:101
E               viewer/__init__.py:6
E               viewer/plugins/base.py:92
E               viewer/plugins/overlayplugin.py:37
E               viewer/utils/core.py:10
E               color/colorlabel.py:159
E               color/colorconv.py:989

hmaarrfk · 2018-10-09T05:25:55Z

@stefanv it seems that the script linked to won't work for us if we use functools.partial.

The use the AST to detect the presence of a stacklevel keyword argument and raise an error if it is missing.

I'm not sure how much you wanted to add that convenience function. If you don't want it, I can take it off.

lagru · 2018-10-09T12:13:43Z

I don't think we should be making our own function for warnings since many contributors might be used to the standard warnings function, and understand what the stacklevel parameter means there.

I agree with this. Furthermore shipping a wrapped function which does nothing but change default values will only increase the mental load and introduce unexpected behavior if one isn't aware of this. It adds (in my opinion) very little value.
Reviewers still have to check manually in both cases whether the stacklevel is correct. Additionally, in case scikit-image ships its own warning function reviewers have to check if the correct warning function is used.

Instead (if considered necessary) I would prefer to see a guideline added here.

Also it might be reasonable to discuss whether scikit-image should raise UserWarnings, library specific warnings or just a plain Warnings. I'd argue for the latter.

lagru · 2018-10-09T12:18:48Z

skimage/util/dtype.py

    return convert(image, np.bool_, force_copy)
+
+
+def convert_to_float(image, preserve_range):


Could you explain why you moved this function? Why include it in this PR?

It caused a circular import when i tried to import _shared.warnings

I think eventually we won't use our own warn function, and just import warnings directly. I'll probably revert this.

I think jni will want to move things around, but we will have to find a more systematic way to do things.

hmaarrfk · 2018-10-09T12:35:06Z

I think a solution that includes the scipy test script is pretty good. it checks if the stacklevel is set to anything at all. For sure the stack level should be greater than 1.

Regarding what kind of warning should be issued, i thought the default warning was UserWarning. I'm totally in favour of keeping it as one of the default warnings.

>>> import warnings
>>> warnings.warn('hello lagru')
__main__:1: UserWarning: hello lagru

lagru · 2018-10-09T12:52:34Z

i thought the default warning was UserWarning. I'm totally in favour of keeping it as one of the default warnings. -- @hmaarrfk

Yes, UserWarning is the default. But I have the impression that that default was chosen with users in mind and not library maintainers. I don't consider libraries shipped via conda or pip as user code and therefore think that scikit-image as a library should refrain from raising UserWarnings.

I'm aware that this is based on my opinion, so our views might differ here. ;)

hmaarrfk · 2018-10-09T13:03:56Z

Oh I see. Yeah. That is a good point. Do we need to chane our testing suite to catch Warnings as well?

My thoughts are that these kinda of fixes can be backported and maybe released more frequently?

…ing imported.

hmaarrfk · 2018-11-16T10:38:26Z

I'm closing this in favour of #3438

The reasoning is that the other one actually stops future contributors from not setting warnings to the correct location.

lagru · 2018-11-16T10:58:32Z

I'm closing this in favour of #3438

You're referencing the PR you just closed here. Did you mean #3467?

hmaarrfk · 2018-11-16T11:00:59Z

Yeah, I totally meant the #3467 too many PRs named the same.Thanks!

hmaarrfk mentioned this pull request Oct 2, 2018

How do I suppress warnings for future deprecation? #3436

Closed

This comment has been minimized.

Sign in to view

hmaarrfk force-pushed the warnings_correct_stack_level branch from 893b452 to 0fb3b4a Compare October 2, 2018 15:56

hmaarrfk mentioned this pull request Oct 2, 2018

Audit our code import structure #3397

Open

5 tasks

hmaarrfk force-pushed the warnings_correct_stack_level branch from 0fb3b4a to 999c699 Compare October 2, 2018 16:04

soupault added the ⏩ type: Enhancement Improve existing features label Oct 5, 2018

soupault added this to the 0.15 milestone Oct 5, 2018

hmaarrfk mentioned this pull request Oct 9, 2018

Improvement: MNT: don't use filter_warnings in test suite. #3459

Merged

4 tasks

hmaarrfk mentioned this pull request Oct 9, 2018

Use stacklevel in warnings #2097

Closed

lagru reviewed Oct 9, 2018

View reviewed changes

hmaarrfk mentioned this pull request Oct 10, 2018

MNT: Set stacklevel manually for warnings. #3467

Closed

9 tasks

hmaarrfk added 3 commits November 3, 2018 09:01

MNT: Set the stack level to the correct location

538e718

MNT: ensure that the warn function is imported from internal _shared

30db707

MNT: Remove circular dependency which stopped _shared.utils from be…

ca95daf

…ing imported.

hmaarrfk force-pushed the warnings_correct_stack_level branch from 999c699 to ca95daf Compare November 3, 2018 13:03

lagru mentioned this pull request Nov 11, 2018

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

Merged

9 tasks

hmaarrfk closed this Nov 16, 2018

		return convert(image, np.bool_, force_copy)


		def convert_to_float(image, preserve_range):

Uh oh!

Conversation

hmaarrfk commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

For reviewers

Uh oh!

This comment has been minimized.

stefanv commented Oct 2, 2018

Uh oh!

hmaarrfk commented Oct 3, 2018

Uh oh!

stefanv commented Oct 4, 2018

Uh oh!

hmaarrfk commented Oct 9, 2018

Uh oh!

hmaarrfk commented Oct 9, 2018

Uh oh!

hmaarrfk commented Oct 9, 2018

Uh oh!

lagru commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lagru Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Oct 9, 2018

Uh oh!

lagru commented Oct 9, 2018

Uh oh!

hmaarrfk commented Oct 9, 2018

Uh oh!

hmaarrfk commented Nov 16, 2018

Uh oh!

lagru commented Nov 16, 2018

Uh oh!

hmaarrfk commented Nov 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hmaarrfk commented Oct 2, 2018 •

edited

Loading

lagru commented Oct 9, 2018 •

edited

Loading