Opened 8 years ago

Closed 8 years ago

#14748 closed defect (fixed)

Deal with DeprecationWarning(s) that appear when using the BoundClass

Reported by: strogdon Owned by: jason
Priority: major Milestone: sage-5.11
Component: misc Keywords:
Cc: nthiery, darij, tscrim Merged in: sage-5.11.rc0
Authors: Steven Trogdon Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by strogdon)

When doctesting Sage with #14136 the following DeprecationWarning appears:

doctest:201: DeprecationWarning: object.__init__() takes no parameters

which was reported on sage-devel (https://groups.google.com/forum/?fromgroups#!topic/sage-devel/wQoCEeKrZ3w). The warning seems to be associated with the use of the BoundClass which inherits from functools.partial (http://trac.sagemath.org/sage_trac/ticket/14136#comment:19). The attached patch is an attempt to work around the warning.

Apply:

Attachments (4)

trac_14748_deprecationwarning.patch (633 bytes) - added by strogdon 8 years ago.
trac_14748_doctest.patch (1.8 KB) - added by strogdon 8 years ago.
updated doctest patch with format changes
trac_14748-review-ts.patch (2.1 KB) - added by tscrim 8 years ago.
trac_14748-deprecationwarning_doctest_reviewer.patch (1.8 KB) - added by strogdon 8 years ago.

Download all attachments as: .zip

Change History (25)

Changed 8 years ago by strogdon

comment:1 Changed 8 years ago by strogdon

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by tscrim

  • Cc nthiery darij tscrim added
  • Type changed from PLEASE CHANGE to defect

comment:3 Changed 8 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

This doesn't seem to break anything, and the failing patchbot test doesn't seem to be related and passed for me:

sage -t --long ../interfaces/expect.py
    [81 tests, 79.66 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 80.3 seconds
    cpu time: 1.6 seconds

So I'm giving this a positive review.

comment:4 Changed 8 years ago by nthiery

  • Status changed from positive_review to needs_work
  • Work issues set to add regression doctest

Thanks for fixing my code; this sounds good! Please add a doctest to prevent a future regression though.

comment:5 Changed 8 years ago by strogdon

Sorry for the delay. I've had limited internet access for the last several days and things will not improve for the next 4 or 5 days. Initially I thought the doctest would be straightforward? However, It's not as easy to catch deprecations with the doctest framework as it is to catch them from the Sage prompt. It may be that bindable_class.py will have to be modified to import the warnings module before a meaningful doctest can be implemented. Thus, I'm not exactly sure what's the best way to proceed. Of course, one could implement a doctest with calls to QuasiSymmetricFunctions that exhibited the initial DeprecationWarning at trac #14136, but I don't see that as failsafe.

comment:6 Changed 8 years ago by darij

Quoth Nicolas (he just asked me to post this quote from one of his emails):

The warning comes when trying to
inherit from it functools.partial:


    sage: import warnings
    sage: warnings.simplefilter('error', DeprecationWarning)
    sage: import functools
    sage: def f(x): return x
    sage: g = functools.partial(f,1)
    sage: g()
    1
    sage: class mypartial(functools.partial):
    ....:     def __init__(self, f, i):
    ....:         functools.partial.__init__(self, f,i)
    sage: g = mypartial(f,1)
    Traceback (most recent call last)
    ...
    ----> 3                 functools.partial.__init__(self, f,i)
    DeprecationWarning: object.__init__() takes no parameters
    sage: g()
    1

I had never noticed this. Please explore for a good workaround!

comment:7 Changed 8 years ago by strogdon

OK, I was doing something just plain wrong. Attached is a patch to include a doctest. Apply this new patch along with the deprecationwarning patch.

comment:8 Changed 8 years ago by strogdon

  • Status changed from needs_work to needs_review

comment:9 Changed 8 years ago by tscrim

  • Work issues add regression doctest deleted

Hey Steven,

Could you please format the doctests in the following format:

    Make sure classes which inherit from functools.partial have the correct syntax, see
    :trac:`14136` and :trac:`14748`::

        sage: import warnings
        sage: warnings.simplefilter('error', DeprecationWarning)
        sage: import functools
        sage: def f(x, y): return x^y
        sage: g = functools.partial(f, 2, 3)
        sage: g()
        8

In particular, there are 2 colons at the end of the text line and a blankline immediately following it. Also for the line above, I autolinked to the trac tickets which uses the syntax :trac:`12345`. Could you fix these?

Thanks,
Travis

comment:10 Changed 8 years ago by strogdon

Thanks Travis. Before I make changes, additionally what is the correct way to format the returned expression

----> 3         functools.partial.__init__(self, f, i, j)

The ----> 3 only appears when things are entered from the Sage prompt. I suspect it is ignored when doctesting. When doctesting a file that which is returned is something like

Traceback (most recent call last):
...
    functools.partial.__init__(self, f, i, j)

Doctesting seems to be insensitive to either of the above when comparing Expected: to Got: as long as there is some whitespace before

functools.partial.__init__(self, f, i, j)

comment:11 Changed 8 years ago by tscrim

Insensitivity to the whitespace is the expected (and often useful) behavior. The correct way to return an error being raised is what you had (but it is generally at the same indentation level):

Traceback (most recent call last):
...
functools.partial.__init__(self, f, i, j)

The most important thing is that all tests pass when running sage -t bindable_class.py.

Best,
Travis

Last edited 8 years ago by tscrim (previous) (diff)

comment:12 Changed 8 years ago by strogdon

A new doctest patch has been uploaded with format changes.

Changed 8 years ago by strogdon

updated doctest patch with format changes

comment:13 Changed 8 years ago by strogdon

Does anyone know why the patchbot is complaining? Is it because of new continuation line syntax - ....: instead of ... or is something else up?

comment:14 Changed 8 years ago by tscrim

  • Description modified (diff)

You are correct, it is because of the line continuation docstring syntax. Here's a review patch which fixes it and does some minor tweaks. If you're happy with my changes, you can set this to positive review.

comment:15 Changed 8 years ago by strogdon

  • Status changed from needs_review to positive_review

Is the indentation level correct on the def line from the reviewer patch?

         sage: class mypartial(functools.partial):
-        ...       def __init__(self, f, i, j):
-        ...           functools.partial.__init__(self, f, i, j)
+        ....:    def __init__(self, f, i, j):
+        ....:         functools.partial.__init__(self, f, i, j)

I'll let you decide. Perhaps it's not important. There are no failures and otherwise it looks good.

Changed 8 years ago by tscrim

comment:16 Changed 8 years ago by tscrim

No, it was one short. Good catch.

comment:17 Changed 8 years ago by strogdon

  • Status changed from positive_review to needs_work

I hadn't checked the revised reviewer patch until today. It appears it has incorrect line endings

file trac_14748-review-ts.patch
trac_14748-review-ts.patch: unified diff output, ASCII text, with CRLF line terminators

and thus fails to apply. If I convert to UNIX everything is fine.

comment:18 Changed 8 years ago by tscrim

That's what I get for adding the space to the patch on a windows machine. Could you fold all of the patches into one patch and (re)upload? Thanks.

comment:19 Changed 8 years ago by strogdon

  • Description modified (diff)
  • Status changed from needs_work to positive_review

Folded patch uploaded. Setting this back to positive review.

comment:20 Changed 8 years ago by darij

patchbot:

apply trac_14748-deprecationwarning_doctest_reviewer.patch

comment:21 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.11.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.