Opened 4 years ago

Closed 3 years ago

#24562 closed defect (fixed)

py3: fixes to sage.misc.decorators

Reported by: embray Owned by:
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: chapoton Merged in:
Authors: Erik Bray Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 44ee2ec (Commits, GitHub, GitLab) Commit: 44ee2ec1bf1c6cd3e1d4654a2ad3696fe3376b2b
Dependencies: Stopgaps:

Status badges

Description


Change History (29)

comment:1 Changed 4 years ago by git

  • Commit set to e7a5e3929ef2108094549a97e63e1dba3fb9b09f

Branch pushed to git repo; I updated commit sha1. New commits:

e7a5e39Python 3 fixes for sage.misc.decorators.

comment:2 Changed 4 years ago by embray

  • Status changed from new to needs_review

comment:3 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_info

As far as I can tell there's only one place in Sage that uses sage_wraps on a class at all

Where is this "one place in Sage"? If you want me to test this, I guess I should know...

comment:4 Changed 4 years ago by chapoton

There is only one place..

git grep -A2 -B2 "@sage_wrap" | grep -B2 "class"
src/sage/misc/decorators.py-
src/sage/misc/decorators.py:        @sage_wraps(func)
src/sage/misc/decorators.py-        class wrapper:

comment:5 Changed 4 years ago by jdemeyer

  • Status changed from needs_info to needs_work

The new version doesn't deal with __doc__ correctly. Try adding this patch:

  • src/sage/misc/decorators.py

    diff --git a/src/sage/misc/decorators.py b/src/sage/misc/decorators.py
    index ab16139..62e8a6a 100644
    a b class infix_operator(object): 
    191191
    192192    An infix dot product operator::
    193193
    194         sage: def dot(a,b): return a.dot_product(b)
     194        sage: def dot(a,b):
     195        ....:     "Dot product"
     196        ....:     return a.dot_product(b)
    195197        sage: dot=infix_operator('multiply')(dot)
    196198        sage: u=vector([1,2,3])
    197199        sage: v=vector([5,4,3])

comment:6 Changed 4 years ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:7 Changed 4 years ago by chapoton

branch does not apply anymore

comment:8 Changed 4 years ago by git

  • Commit changed from e7a5e3929ef2108094549a97e63e1dba3fb9b09f to 0bb9b1a7c42e21b926e8546df179d1fff4ff1380

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0bb9b1aPython 3 fixes for sage.misc.decorators.

comment:9 Changed 4 years ago by embray

Rebased. I still need to see about addressing Jeroen's comment.

comment:10 Changed 4 years ago by embray

I'm thinking sage_wraps shouldn't actually work on classes (the fact that it happens to work on old-style classes was kind of a lucky accident. The original functools.wraps doesn't work on classes, and was never designed to either, and sage_wraps isn't documented to work with classes. It was just used in one case (on an old-style class) and it happened to work.

It's possible to implement wraps-like functionality for classes but also a bit trickier (unfortunately). I think that would be outside the scope of this ticket.

comment:11 Changed 4 years ago by git

  • Commit changed from 0bb9b1a7c42e21b926e8546df179d1fff4ff1380 to 1f61a71b8b899cec2a4477f171e5135540b7140f

Branch pushed to git repo; I updated commit sha1. New commits:

19f3d11dont' try to make sage_wraps work on classes at all
1f61a71for compatibility with Python 3's functools.wraps, set __wrapped__ attribute on the wrapper function

comment:12 Changed 4 years ago by git

  • Commit changed from 1f61a71b8b899cec2a4477f171e5135540b7140f to 44ee2ec1bf1c6cd3e1d4654a2ad3696fe3376b2b

Branch pushed to git repo; I updated commit sha1. New commits:

44ee2ecclean up the infix_operator docstrings a bit to demonstrate actual use as a decorator

comment:13 Changed 4 years ago by embray

Added a little cleanup on the tests for infix_operator. Perhaps should add more tests, but unique tests :)

comment:14 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

comment:15 Changed 4 years ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to needs_work

I believe that by our policy the methods in here should have doctests. Or is there some reason why they shouldn't?

comment:16 follow-up: Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

What methods do you think require tests? These are mostly internal details that are tested implicitly by the existing examples.

comment:17 Changed 4 years ago by embray

  • Milestone changed from sage-8.3 to sage-8.4

I believe this issue can reasonably be addressed for Sage 8.4.

comment:18 Changed 4 years ago by embray

The failures are in -optional: magma tests that I guess are enabled on that particular buildbot. Nothing to do with this I don't think.

comment:19 in reply to: ↑ 16 Changed 4 years ago by saraedum

  • Status changed from needs_review to needs_work

Replying to embray:

What methods do you think require tests? These are mostly internal details that are tested implicitly by the existing examples.

Sorry, but I don't understand what "internal details" means. I know we had this discussion before but unless something has changed, virtually all methods should be doctested, e.g., the ones in _infix_wrapper. Personally, I have no opinion on whether that's a good or a bad policy (I can see valid arguments on both sides, so I am not sure) but I think it's still the way things work; which could be changed of course.

Btw, with the current state of documentation it is not immediately clear to me what _infix_wrapper does. A docstring that explains what this class is doing would be helpful.

comment:20 follow-up: Changed 4 years ago by embray

It doesn't always make sense to write examples for methods that merely serve some internal purpose and are never used directly--instead the tests use these things indirectly in the process of doing the thing it is they're supposed to do. What this leads to is tests like you can see in the original code, such as:

-        def left_func(self, right):
-            """
-            The function for the operation on the left (e.g., __add__).
-
-            EXAMPLES::
-
-                sage: def dot(a,b): return a.dot_product(b)
-                sage: dot=infix_operator('multiply')(dot)
-                sage: u=vector([1,2,3])
-                sage: v=vector([5,4,3])
-                sage: u *dot* v
-                22
-            """

This exact same test is repeated like 3 or 4 times in that module, because someone said "it needs an example block" in every method. This should be clarified in the developer docs, but that really does not mean "literally every def statement". The intention is that every function or method that someone actually uses directly is documented in this way.

Sometimes it still makes sense to have as an example of how an internal method is used, if it's unclear, or if it's an internal method that might be useful when adding new methods to a class or subclass. But not if it means going into contortions to set up a test for a method that is never called directly in the first place.

comment:21 follow-up: Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

Anyways, this ticket isn't adding particularly new code; just slightly refactoring existing code that was written in an odd way that made it needlessly non-portable. Needlessly duplicated doctests are also removed.

If it's unclear, _infix_wrapper is just a base class for wrappers around a function turning that function into an infix operator. infix_operator itself is just the decorator, but _infix_wrapper is the base for the actual wrapped functions returned by the decorator.

This was already in the original code under just the name "wrapper", and it was a class defined inline in infix_operator.__call__. I just moved it out to module level because there was no-need to create new instances of that base class for every infix_operator call.

comment:22 in reply to: ↑ 20 Changed 4 years ago by saraedum

Replying to embray:

[...] This exact same test is repeated like 3 or 4 times in that module, because someone said "it needs an example block" in every method. This should be clarified in the developer docs, but that really does not mean "literally every def statement". The intention is that every function or method that someone actually uses directly is documented in this way.

I don't agree. Also internal methods should be documented that way wherever possible imo.

Sometimes it still makes sense to have as an example of how an internal method is used, if it's unclear, or if it's an internal method that might be useful when adding new methods to a class or subclass. But not if it means going into contortions to set up a test for a method that is never called directly in the first place.

When I'm not lazy, I try to do an "indirect doctest" when it's too much code to setup the situation where the method could be used or when the method is cleary just a backing for something else, e.g., I'd test _add_ with a+b even though it that calls __add__.

comment:23 in reply to: ↑ 21 Changed 4 years ago by saraedum

Replying to embray:

Anyways, this ticket isn't adding particularly new code; just slightly refactoring existing code that was written in an odd way that made it needlessly non-portable. Needlessly duplicated doctests are also removed.

Sure, the doctests were bad. I think we should have tried to replace them with better doctests instead of no doctests.

If it's unclear, _infix_wrapper is just a base class for wrappers around a function turning that function into an infix operator. infix_operator itself is just the decorator, but _infix_wrapper is the base for the actual wrapped functions returned by the decorator.

Ok. This wasn't clear to me. That would almost be a valid docstring for the class.

So, I'll stop bugging you here I think as this probably won't lead anywhere. I am afraid I can't set this to positive review as is. If somebody else wants to do that I am not opposed at all.

comment:24 Changed 4 years ago by saraedum

  • Reviewers Julian Rüth deleted

comment:25 Changed 4 years ago by embray

  • Cc chapoton added

I'm not sure why you can't. Do you have some concrete suggestion for how this could be documented better, other than adding pointless repetitive doctests?

comment:26 Changed 4 years ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:27 Changed 3 years ago by embray

  • Milestone changed from sage-8.5 to sage-8.7

Retargeting some of my tickets (somewhat optimistically for now).

comment:28 Changed 3 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok. I would have prefered to see stupid doctests everywhere, but let us move on.

comment:29 Changed 3 years ago by vbraun

  • Branch changed from u/embray/python3/sage-misc-decorators to 44ee2ec1bf1c6cd3e1d4654a2ad3696fe3376b2b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.