Opened 3 years ago

Closed 3 years ago

#24119 closed enhancement (fixed)

Create an assuming() context manager

Reported by: charpent Owned by:
Priority: major Milestone: sage-8.1
Component: symbolics Keywords:
Cc: rws Merged in:
Authors: Emmanuel Charpentier Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 017f67c (Commits, GitHub, GitLab) Commit: 017f67c66da8aadfaefebb976528abacef6b58ad
Dependencies: Stopgaps:

Status badges

Description

Inspiration : Ralf Stephan in #22322. Thanks to him for nudging me in the right direction.

Goal : create a tool allowing to create reusable, stackable, assumption elements, thus allowing to get the functionality of Mathematica's Assuming[...,???]

What I mean :

sage: solve(x^2==4, x)
[x == -2, x == 2]
sage: with assuming(x>0): solve(x^2==4, x)
[x == 2]
sage: assumptions()
[]

Change History (21)

comment:1 Changed 3 years ago by charpent

  • Branch set to u/charpent/create_an_assuming___context_manager

comment:2 Changed 3 years ago by charpent

  • Authors set to Emmanuel Charpentier
  • Commit set to 4b5f2ea72349f5caffa596762d2a833be6e5e121
  • Status changed from new to needs_review

Initial proposition.

On 8.1beta9 + #24026+ #24006 + #23990 + #23923 + #24088 + #10035, passes ptestlong with no error whatsoever (1 timeout once on sage -t --long src/sage/modular/hecke/submodule.py, unreproductible).

==>needs_review

comment:3 follow-up: Changed 3 years ago by charpent

  • Status changed from needs_review to needs_work

Aaarghh... I've found a subtle bug: after with assuming(): do_something(), the original assumption database is destroyed.

That's due to the fact that forget() is equivalent to forget(assumptions()), i. e. forgets all facts in the database. Hoisted by a "lazy" facility...

I'll fix that (and add a couple doctests for exceptions.

Should we reconsider forget() <==> forget(assumptions()) ?

comment:4 follow-up: Changed 3 years ago by git

  • Commit changed from 4b5f2ea72349f5caffa596762d2a833be6e5e121 to c50defe06a74c6ead95ed63bb319eb549aed1d15

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

c50defe24119 : Debugging, better doctests, a couple of typos

comment:5 in reply to: ↑ 4 Changed 3 years ago by charpent

  • Status changed from needs_work to needs_review

Replying to git:

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

c50defe24119 : Debugging, better doctests, a couple of typos

Passes ptestlong with no errors whatsoever, an one non-reproducible timeout on sage -t --long src/sage/symbolic/expression.pyx.

==>needs_review again.

comment:6 in reply to: ↑ 3 Changed 3 years ago by charpent

Replying to charpent:

Aaarghh... I've found a subtle bug: after with assuming(): do_something(), the original assumption database is destroyed.

[ Snip ... ]

To be absolutely clear : the problem existed when one used assuming with an empty argument list : you wouldn't do that from the console, but this argument list might result from another computation, which might return an empty list.

Anyway, problem fixed.

comment:7 follow-up: Changed 3 years ago by rws

  • Reviewers set to Ralf Stephan

Could you please add direct doctests to the member functions?

comment:8 follow-up: Changed 3 years ago by rws

Also add a doctest with the class that checks correctness with at least doubly stacked assumptions.

comment:9 in reply to: ↑ 7 Changed 3 years ago by charpent

Replying to rws:

Could you please add direct doctests to the member functions?

What do you mean ? I added exactly ONE function (assuming()), which I documented and doctested, and added a small blurb at the end of the module documentation. Since assuming() is designed to interact exclusively with assume(), I don't see what you mean.

comment:10 in reply to: ↑ 8 Changed 3 years ago by charpent

Replying to rws:

Also add a doctest with the class that checks correctness with at least doubly stacked assumptions.

This I understand and will do. Stay tuned...

comment:11 Changed 3 years ago by rws

The class assuming has three member functions, no? Check class hold_class in #10035 where __enter__ and __exit__ is doctested.

comment:12 Changed 3 years ago by git

  • Commit changed from c50defe06a74c6ead95ed63bb319eb549aed1d15 to a43a32aa5a6b4627aef6067c5e606f20206a0c8a

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

a43a32a24119 : doctest for stacks of assumptions.

comment:13 follow-up: Changed 3 years ago by git

  • Commit changed from a43a32aa5a6b4627aef6067c5e606f20206a0c8a to 127d507348ca429602585bf0734e2741d889b135

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

127d50724119 : doctesting individual assuming's member functions.

comment:14 in reply to: ↑ 13 Changed 3 years ago by charpent

Replying to git:

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

127d50724119 : doctesting individual assuming's member functions.

Builds and doctests OK. make ptestlong underway (don't hold your breath...).

comment:15 Changed 3 years ago by rws

I like it. Here some minor typos: double space after declaration; :func:assume()) should have backticks; typo in useable in a in a with statement; small caps begin new sentence in ...can be stacked. we can use this functionality can be used to... also use is used twice; please correct (and by yhe way,; finally please add an empty line before def __enter__ and before def __exit__.

comment:16 Changed 3 years ago by charpent

Passes ptestlong with no error whatsoever.

I'll fix the typos and re-upload, but won't re-ptestlong before late tonight (I *have* to go now...).

comment:17 follow-up: Changed 3 years ago by git

  • Commit changed from 127d507348ca429602585bf0734e2741d889b135 to 017f67c66da8aadfaefebb976528abacef6b58ad

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

017f67c24119 : small typos fixed.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by charpent

Replying to git:

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

017f67c24119 : small typos fixed.

Typos fixed, except a double space after declaration, which I didn't find. Builds and doctests OK. ptestlong later tonight. Leaving NOW.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by charpent

Replying to charpent:

Replying to git:

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

017f67c24119 : small typos fixed.

Typos fixed, except a double space after declaration, which I didn't find. Builds and doctests OK. ptestlong later tonight. Leaving NOW.

[ Later... ]

  • Passes ptestlong with no error whatsoever.
  • Most of the 17 patchbots seem to like it, but all but one of them complain that "a plugin failed" ; it seems that this is the "coverage" plugin. Not my watch, I'm afraid...
  • Four patchbots got a failure or a timeout :
    • timeout in src/sage/interfaces/giac.py : doesn't seem related to my ticket ;
    • one failure on src/sage/doctest/external.py : ditto ;
    • timeout in src/sage/graphs/graph_generators.py : ditto ;
    • one failure in src/sage/schemes/elliptic_curves/heegner.py and one timeout in src/sage/coding/linear_code.py : ditto.

These failures seems "too random" to have a systematic cause, and may result from a race condition somewhere in the test infrastructure. We have been plagued with such "transient" failures for some time now.

Seems good to review...

comment:20 in reply to: ↑ 19 Changed 3 years ago by rws

  • Status changed from needs_review to positive_review

Replying to charpent:

  • Most of the 17 patchbots seem to like it, but all but one of them complain that "a plugin failed" ; it seems that this is the "coverage" plugin. Not my watch, I'm afraid...

The coverage plugin (of patchbot) looks if you added code without doctest. It uses the call sage --coverage which you can do too. You fixed it however when I asked you so the recent patchbot runs no longer show it. It's all fine now.

comment:21 Changed 3 years ago by vbraun

  • Branch changed from u/charpent/create_an_assuming___context_manager to 017f67c66da8aadfaefebb976528abacef6b58ad
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.