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

Issue @experimental warnings only once #20601

Closed
johanrosenkilde opened this issue May 13, 2016 · 30 comments
Closed

Issue @experimental warnings only once #20601

johanrosenkilde opened this issue May 13, 2016 · 30 comments

Comments

@johanrosenkilde
Copy link
Contributor

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)

CC: @sagetrac-dlucas @dkrenn @sagetrac-tmonteil

Component: misc

Keywords: warnings, experimental

Author: Johan Sebastian Rosenkilde Nielsen

Branch: 7298382

Reviewer: David Lucas

Issue created by migration from https://trac.sagemath.org/ticket/20601

@johanrosenkilde
Copy link
Contributor Author

Branch: u/jsrn/20601_experimental

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2016

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

f967f59Make @experimental warnings issue only once

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2016

Commit: f967f59

@johanrosenkilde
Copy link
Contributor Author

comment:3

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)

@johanrosenkilde
Copy link
Contributor Author

Author: Johan S. R. Nielsen

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2016

Changed commit from f967f59 to 5c28822

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2016

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

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

@johanrosenkilde
Copy link
Contributor Author

comment:5

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.

@jdemeyer
Copy link
Contributor

comment:6

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2016

Changed commit from 5c28822 to d14bdd4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2016

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

d14bdd4Sphinx link for 20601 and __experimental_self_test

@johanrosenkilde
Copy link
Contributor Author

comment:8

Done.

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented May 13, 2016

comment:9

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2016

Changed commit from d14bdd4 to 29095e5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2016

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

29095e5Fix stupid typo

@johanrosenkilde
Copy link
Contributor Author

comment:11

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

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented May 13, 2016

comment:12

Great, I'm giving the green light!

David

@johanrosenkilde
Copy link
Contributor Author

comment:13

Awesome, thanks!

@tscrim
Copy link
Collaborator

tscrim commented May 13, 2016

comment:14

Reviewer name(s)

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented May 13, 2016

Reviewer: David Lucas

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented May 13, 2016

comment:16

Reviewer name(s)

Oops. Done now!

@vbraun
Copy link
Member

vbraun commented May 20, 2016

comment:17

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]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2016

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

7298382Rm some TESTS meant for old warning behaviour

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2016

Changed commit from 29095e5 to 7298382

@johanrosenkilde
Copy link
Contributor Author

comment:19

Indeed, fixed.

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented May 24, 2016

comment:20

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

David

@vbraun
Copy link
Member

vbraun commented May 24, 2016

Changed branch from u/jsrn/20601_experimental to 7298382

@fchapoton
Copy link
Contributor

Changed author from Johan S. R. Nielsen to Johan Sebastian Rosenkilde Nielsen

@fchapoton
Copy link
Contributor

Changed commit from 7298382 to none

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 30, 2020

comment:23

Follow-up: #29272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants