Opened 4 years ago

Closed 4 years ago

Last modified 7 months ago

#20601 closed defect (fixed)

Issue @experimental warnings only once

Reported by: jsrn Owned by:
Priority: major Milestone: sage-7.3
Component: misc Keywords: warnings, experimental
Cc: dlucas, dkrenn, tmonteil Merged in:
Authors: Johan Sebastian Rosenkilde Nielsen Reviewers: David Lucas
Report Upstream: N/A Work issues:
Branch: 7298382 (Commits) Commit:
Dependencies: Stopgaps:

Description

Marking a function/method/constructor as @experimental causes the experimental warning message to be issued every time. This seems to be due to a general convention in Sage that warnings are issued repeatedly.

That convention leaves @experimental virtually useless, however, since the user is clearly aware of the experimental nature of the code after the first issuing of the warning, but nonetheless chooses to keep on using it.

This patch makes sure that the @experimental warnings are issued only once (without changing behaviour for other warnings)

Change History (23)

comment:1 Changed 4 years ago by jsrn

  • Branch set to u/jsrn/20601_experimental

comment:2 Changed 4 years ago by git

  • Commit set to f967f59b5e9bb76aa6dd96fda7c578f7a64808d0

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

f967f59Make @experimental warnings issue only once

comment:3 Changed 4 years ago by jsrn

  • Authors set to Johan S. R. Nielsen
  • Status changed from new to needs_review

I've implemented a flag so @experimental knows whether it has already issued a warning or not. The flag works as expected in the terminal.

However, during doc-testing I get strange behaviour: I wanted to add a test to demonstrate that an error message is now only issued on the first call. However, the test passes even without the patch! This is in stark contrast to the behaviour in the terminal, where the message (copy-paste the code from the doc-tested) is definitely issued every time. There is some magic in either Sphinx or the terminal interface that I'm not understanding here.

(more data is that #20526, commit 0ab93ec currently has all doc-tests failing due to experimental warnings being issued, but with this patch, all the tests pass)

comment:4 Changed 4 years ago by git

  • Commit changed from f967f59b5e9bb76aa6dd96fda7c578f7a64808d0 to 5c288220006c7d1edd93594b0b20ac6dbedfe6eb

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

5c28822Added a test that in doc-tests demonstrates the behaviour

comment:5 Changed 4 years ago by jsrn

I managed to make a test that properly demonstrates the behaviour: without the patch, the test fails since extra FutureWarnings are issued, while the test passes with the patch.

Due to aforementioned Sphinx magic the test is a bit complicated: it seems the test has to involve a "real" function/class and span multiple documentation strings, so I've added a class __experimental_self_test for this purpose.

Note that this behaviour is really critical, i.e. that only a single warning is issued across documentation strings: if not, a class that is marked as @experimental would need to add the FutureWarning to *all* documentation strings in the entire file. With this patch, only a single at the top needs to be added (e.g. together with the documentation text that describes that the functionality is experimental).

The patch is still in needs review.

comment:6 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Detail: replace #20601 by :trac:`20601` in docstrings.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:7 Changed 4 years ago by git

  • Commit changed from 5c288220006c7d1edd93594b0b20ac6dbedfe6eb to d14bdd40b8aeb1091b67a1c10ae6da2a54d2ecbb

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

d14bdd4Sphinx link for 20601 and __experimental_self_test

comment:8 Changed 4 years ago by jsrn

  • Status changed from needs_work to needs_review

Done.

comment:9 Changed 4 years ago by dlucas

Hello,

It works (I tested it in #20526 and it fixed my problem with experimental warnings), the doc looks good... But does not compile because you wrote :method: instead of :meth: l. 222 in superseded.py.

If you fix that I'll set this ticket to positive review.

comment:10 Changed 4 years ago by git

  • Commit changed from d14bdd40b8aeb1091b67a1c10ae6da2a54d2ecbb to 29095e57d7f06657aa71b2bf29b892589b1216ae

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

29095e5Fix stupid typo

comment:11 Changed 4 years ago by jsrn

Stupid of me: I forgot ./sage -b before recompiling the doc and hence didn't see the error. Fixed now.

comment:12 Changed 4 years ago by dlucas

  • Status changed from needs_review to positive_review

Great, I'm giving the green light!

David

comment:13 Changed 4 years ago by jsrn

Awesome, thanks!

comment:14 Changed 4 years ago by tscrim

Reviewer name(s)

comment:15 Changed 4 years ago by dlucas

  • Reviewers set to David Lucas

comment:16 Changed 4 years ago by dlucas

Reviewer name(s)

Oops. Done now!

comment:17 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Various doctest failures, e.g.

sage -t --long --warn-long 40.7 src/sage/rings/asymptotic/asymptotic_ring.py
**********************************************************************
File "src/sage/rings/asymptotic/asymptotic_ring.py", line 109, in sage.rings.asymptotic.asymptotic_ring
Failed example:
    R.<x, y> = AsymptoticRing(growth_group='x^ZZ * y^ZZ', coefficient_ring=ZZ)
Expected:
    doctest:...: FutureWarning: This class/method/function is marked as
    experimental. It, its functionality or its interface might change
    without a formal deprecation.
    See http://trac.sagemath.org/17601 for details.
Got:
    <BLANKLINE>
**********************************************************************
1 item had failures:
   1 of  44 in sage.rings.asymptotic.asymptotic_ring
    [583 tests, 1 failure, 7.26 s]

comment:18 Changed 4 years ago by git

  • Commit changed from 29095e57d7f06657aa71b2bf29b892589b1216ae to 7298382019b11f69a3bb4d39942591f3fff377f7

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

7298382Rm some TESTS meant for old warning behaviour

comment:19 Changed 4 years ago by jsrn

  • Status changed from needs_work to needs_review

Indeed, fixed.

comment:20 Changed 4 years ago by dlucas

  • Status changed from needs_review to positive_review

I ran the tests and it works on my side, setting to positive_review.

David

comment:21 Changed 4 years ago by vbraun

  • Branch changed from u/jsrn/20601_experimental to 7298382019b11f69a3bb4d39942591f3fff377f7
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 4 years ago by chapoton

  • Authors changed from Johan S. R. Nielsen to Johan Sebastian Rosenkilde Nielsen
  • Commit 7298382019b11f69a3bb4d39942591f3fff377f7 deleted

comment:23 Changed 7 months ago by mkoeppe

Follow-up: #29272

Note: See TracTickets for help on using tickets.