Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: using python's warnings instead of prints in fitpack. #4003

Merged
merged 3 commits into from
Sep 25, 2014

Conversation

Dapid
Copy link
Contributor

@Dapid Dapid commented Sep 18, 2014

When fitpack detects a recoverable problem, it will signal it with
a RuntimeWarning instead of printing to stdout. This way the output
can be programatically silenced or made exceptions.

There is one failure on Travis, but I don't see what has failed.

When fitpack detects a recoverable problem, it will signal it with
a RuntimeWarning instead of printing to stdout. This way the output
can be programatically silenced or made exceptions.
@WarrenWeckesser
Copy link
Member

The failure of job 3059.3 is the 50 minute timeout that we've been seeing regularly. The failure of 3059.1 is something new. It looks like Travis stops the build when the number of output lines hits 10000. I got the same failure with #4004

@insertinterestingnamehere
Copy link
Member

The Travis build only shows the first 10000 lines of output in their main view. If you view the log as a text file, it will show the remaining lines. The link for that is on the right side a little down from the top. It has four horizontal bars. The error shown in both of those build failures is

scipy/ndimage/src/ni_filters.c:386:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]

@WarrenWeckesser
Copy link
Member

@insertinterestingnamehere Thanks! I was just about to update my comment to note that the log might be truncated. Good to know that is actually what is happening.

@insertinterestingnamehere
Copy link
Member

No problem. Looking back, I think merging #3527 broke this. That's just a guess though.

@WarrenWeckesser
Copy link
Member

Yup--you're one step ahead of me again. :)

@Dapid
Copy link
Contributor Author

Dapid commented Sep 19, 2014

Should I add some tests to ensure the warnings are raised? Ot is it too runtime dependent?

Now I think about it, the docstrings should be modified too. What about this:

    quiet : bool
        Non-zero to suppress messages.

becoming this?

    quiet : bool
        Non-zero to suppress messages. Else, they will be issued as `RuntimeWarnings`.

@ev-br
Copy link
Member

ev-br commented Sep 19, 2014

You can add a test with mest=0, so that the intention is clear. Re: docstring, I'd actually add that this parameter is deprecated and we suggest its not used in new code (use standard python warnings filters instead)

@ev-br ev-br added maintenance Items related to regular maintenance tasks scipy.interpolate labels Sep 23, 2014
@@ -21,12 +21,13 @@
For univariate splines: cocosp, concon, fourco, insert
For bivariate splines: profil, regrid, parsur, surev
"""
from __future__ import division, print_function, absolute_import
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: IMO it's better to keep importing print_function even if neither print() function nor print statement should be used in the library code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is that?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is boilerplate at the top of every python file for python3 compatibility. I think there's even a bot that enforces it, or that added them at some point.

@ev-br
Copy link
Member

ev-br commented Sep 23, 2014

LGTM modulo nitpicks. Adding a test for mest=0 is not essential I think.

Restarted the tests, TravisCI is green.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling ad056f9 on Dapid:warnings into fdddfc3 on scipy:master.

@Dapid
Copy link
Contributor Author

Dapid commented Sep 25, 2014

I have edited the docstrings and put back the import. I am not sure about how to do the mest=0 test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling ae276ab on Dapid:warnings into fdddfc3 on scipy:master.

@ev-br
Copy link
Member

ev-br commented Sep 25, 2014

There's always something as simple as assert_warns and

>>> x = np.arange(8)
>>> y = 2.*x - 3
>>> tck = splrep(x, y)
>>> sproot(tck, mest=0)
Warning: the number of zeros exceeds mest
array([], dtype=float64)

But IMO this is borderline useful at best.

TravisCI is green, merging. Thanks @Dapid, Warren, Ian, Alex.

ev-br added a commit that referenced this pull request Sep 25, 2014
ENH: using python's warnings instead of prints in fitpack.
@ev-br ev-br merged commit e431c97 into scipy:master Sep 25, 2014
@ev-br ev-br added this to the 0.15.0 milestone Sep 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.interpolate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants