Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#9579 closed defect (fixed)

Raise an exception when arguments to add_constraint are not admissible

Reported by: ncohen Owned by: jason, jkantor
Priority: major Milestone: sage-4.5.2
Component: numerical Keywords:
Cc: Merged in: sage-4.5.2.alpha1
Authors: Nathann Cohen Reviewers: Harald Schilly
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Because of the error reported on :

http://groups.google.com/group/sage-support/browse_thread/thread/b953192f3554337f

It may be good to save an user several hours of trouble because a wrong argument is silently accepted.

Done in this patch ! :-)

Nathann

Attachments (2)

trac_9579_review.patch (837 bytes) - added by schilly 9 years ago.
test both arguments, not only one of them
trac_9579.patch (1.7 KB) - added by ncohen 9 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by ncohen

  • Status changed from new to needs_review

comment:2 follow-up: Changed 9 years ago by schilly

I don't know if testing with RR is the best way to do this, but should work. What's missing is a test for max b/c you only tested for min.

Changed 9 years ago by schilly

test both arguments, not only one of them

comment:3 in reply to: ↑ 2 Changed 9 years ago by ncohen

Replying to schilly:

I don't know if testing with RR is the best way to do this, but should work. What's missing is a test for max b/c you only tested for min.

You do not begin to know how I *HATE* this RR... Is there any way to check whether a value is numerical without having to import RINGS ? :-D

Even a Python method is fine !!! The most esoteric thing that could be found there is a rational number !

Nathann

Changed 9 years ago by ncohen

comment:4 Changed 9 years ago by ncohen

I just updated my patch so that instead of ugly "try/except", a "if" is enough, thanks to Harald's suggestion. By the way, your patch still applies on top of mine, so if you are ok with these changes, let's say this ticket is positively reviewed :-)

Nathann

comment:5 Changed 9 years ago by schilly

there is still this hurdle to run all tests, i'll start them right now.

comment:6 follow-up: Changed 9 years ago by schilly

  • Authors set to Nathan Cohen
  • Reviewers set to Harald Schilly
  • Status changed from needs_review to positive_review

dear release manager, first merge trac_9579.patch and then trac_9579_review.patch

comment:7 in reply to: ↑ 6 Changed 9 years ago by ddrake

  • Merged in set to sage-4.5.2.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

Replying to schilly:

dear release manager, first merge trac_9579.patch and then trac_9579_review.patch

Merged in 4.5.2.alpha1.

comment:8 Changed 9 years ago by mpatel

  • Authors changed from Nathan Cohen to Nathann Cohen

comment:9 Changed 9 years ago by ncohen

My 'n' !! I hadn't noticed it !! Thank you :-)

Nathann

Note: See TracTickets for help on using tickets.