Skip to content

[MNT] Fix test warnings in scripting part#3503

Merged
lanzagar merged 14 commits intobiolab:masterfrom
janezd:fix-test-warning-scripting
Jan 29, 2019
Merged

[MNT] Fix test warnings in scripting part#3503
lanzagar merged 14 commits intobiolab:masterfrom
janezd:fix-test-warning-scripting

Conversation

@janezd
Copy link
Copy Markdown
Contributor

@janezd janezd commented Dec 26, 2018

Similar to #3502, this PR deals with most warnings in the scripting part, except those in postgresql and a few in the code I don't sufficiently understand.

@janezd janezd force-pushed the fix-test-warning-scripting branch 2 times, most recently from 9350394 to 595137c Compare December 26, 2018 23:30
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 26, 2018

Codecov Report

Merging #3503 into master will decrease coverage by 0.04%.
The diff coverage is 96.83%.

@@            Coverage Diff             @@
##           master    #3503      +/-   ##
==========================================
- Coverage   83.59%   83.54%   -0.05%     
==========================================
  Files         367      367              
  Lines       65580    65784     +204     
==========================================
+ Hits        54823    54961     +138     
- Misses      10757    10823      +66

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 26, 2018

Codecov Report

Merging #3503 into master will decrease coverage by <.01%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master    #3503      +/-   ##
==========================================
- Coverage   83.97%   83.97%   -0.01%     
==========================================
  Files         370      370              
  Lines       66920    66941      +21     
==========================================
+ Hits        56196    56212      +16     
- Misses      10724    10729       +5

@janezd janezd force-pushed the fix-test-warning-scripting branch from 595137c to 64cd571 Compare December 26, 2018 23:33
@janezd janezd changed the title [MNT] Fix test warnings in scripting part [WIP] [MNT] Fix test warnings in scripting part Jan 17, 2019
@janezd janezd force-pushed the fix-test-warning-scripting branch 5 times, most recently from 80e6937 to 72b2f9c Compare January 18, 2019 12:57
@janezd janezd changed the title [WIP] [MNT] Fix test warnings in scripting part [MNT] Fix test warnings in scripting part Jan 18, 2019
@janezd janezd force-pushed the fix-test-warning-scripting branch from 72b2f9c to d474b22 Compare January 18, 2019 14:59
@janezd janezd force-pushed the fix-test-warning-scripting branch 2 times, most recently from 4258cad to 730c346 Compare January 25, 2019 22:17
@janezd
Copy link
Copy Markdown
Contributor Author

janezd commented Jan 26, 2019

Build crashed (output too large) after removing the fix for np.sum(generator). Now I got to the bottom of it.

"once" filter in warnings has a nice undocumented behaviour: if the list of filters is changed, the registry of printed warnings is cleared. Hence any call to filterwarnings, even within catch_warnings context, resets the registry.

The problem in this PR is silencing the warnings in RowInstance.__init__ which resets the filters at every iteration of the loop over all instances in naive Bayesian classifier.

If you agree, I'll:

  • I'll leave silencing of warnings within tests. It's harmless since the unittest framework resets the filters anyway.

  • Almost all filters in actual code silence the scikit-sparse's use of matrix subclass. I would disable this warning for entire Orange, that is, put this filter to Orange.__init__. Benefits:

    • It avoids randomly resetting the filters.
    • We never want to see this warning anyway because it's never our fault and we can do nothing about it.
    • It will be more trivial to remove when it's fixed in scikit-sparse
  • I will try to remove other filters, either by

    • fixing the code so they're not issued or
    • using np.errstate instead or
    • keeping filters, if they are unlikely to cause harm,

    or I may keep them, but hesitantly unless we expect them to go away some day.

When/If I get a green light from you, I'll first fix this PR and then do the same in #3502 (there seem to be just six places, also thanks to your cautious approach reviewing it).

@janezd janezd changed the title [MNT] Fix test warnings in scripting part [WIP] [MNT] Fix test warnings in scripting part Jan 26, 2019
@lanzagar
Copy link
Copy Markdown
Contributor

At least now we know why filtering out some warnings can actually lead to more of them in the output!
I agree with all three points, and am a big fan of "fixing the code so they're not issued" approach anyway :)

@janezd janezd force-pushed the fix-test-warning-scripting branch 3 times, most recently from d4e4922 to 6f572b2 Compare January 28, 2019 21:38
@janezd janezd force-pushed the fix-test-warning-scripting branch from 6f572b2 to b10a5e3 Compare January 28, 2019 22:08
@janezd janezd changed the title [WIP] [MNT] Fix test warnings in scripting part [MNT] Fix test warnings in scripting part Jan 28, 2019
Copy link
Copy Markdown
Contributor Author

@janezd janezd left a comment

Choose a reason for hiding this comment

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

I disabled warning about (scipy.sparse's) using of matrix for all Orange. Warnings are silenced on Travis and when tests are run locally in certain ways, but not always: if tests are run in PyCharm, warnings are re-enabled by the test runner. I don't see a good way of disabling it without hacking into the runner or warnings filter. This is not critical, though.

means = np.nanmean(x, axis=0)
vars = np.nanvar(x, axis=0)
if self.normalize and not vars.all():
raise ValueError("some columns are constant")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

x[~mask] unnecessarily copies the array, and I wouldn't like to do it column by column. This patching is a non-idiomatic substitute for catch_warnings.

mads = np.zeros(len(x))
else:
medians = np.nanmedian(x, axis=0)
mads = np.nanmedian(np.abs(x - medians), axis=0)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The warning here was "mean of empty slice", which appeared because median fell back to mean if size equalled 0 (achieving what?!)

# This warning filter can be removed in scikit 0.22
with warnings.catch_warnings():
warnings.filterwarnings(
"ignore", "The behavior of AMI will change in version 0\.22.*")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kept this catch_warnings although it's not in tests. It's supposed to be temporary.

@janezd janezd force-pushed the fix-test-warning-scripting branch from b10a5e3 to bc99d0c Compare January 29, 2019 13:11
@lanzagar lanzagar merged commit 2124dba into biolab:master Jan 29, 2019
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.

2 participants