Opened 11 months ago
Closed 4 months ago
#31414 closed enhancement (fixed)
validate input arguments of elimination_ideal
Reported by:  ghmwageringel  Owned by:  

Priority:  minor  Milestone:  sage9.5 
Component:  commutative algebra  Keywords:  
Cc:  ghDavidAyotte  Merged in:  
Authors:  Markus Wageringel  Reviewers:  David Ayotte 
Report Upstream:  N/A  Work issues:  
Branch:  28c1e2a (Commits, GitHub, GitLab)  Commit:  28c1e2a41633178a4c62d423c999c948a920a26c 
Dependencies:  #29979  Stopgaps: 
Description
As explained in 31367#comment:1, this ticket adds a check that all the arguments passed to elimination_ideal
are actually variables of the ring, to avoid silent problems like this:
age: R.<x,y,z> = QQ[] sage: R.ideal(xy, z).elimination_ideal([y, R(0)]) Ideal (x  y, z) of Multivariate Polynomial Ring in x, y, z over Rational Field
Change History (7)
comment:1 Changed 11 months ago by
 Branch set to u/ghmwageringel/31414
 Commit set to 8ad70e6c63285b65b57b257c7f77d3a404173ad7
 Status changed from new to needs_review
comment:2 Changed 10 months ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:3 followup: ↓ 5 Changed 7 months ago by
 Cc ghDavidAyotte added
 Reviewers set to David Ayotte
 Status changed from needs_review to positive_review
Everything looks good. I ran the doctest and everything passed except for an unrelated test in src/sage/rings/number_field/number_field_ideal.py which also fail on the develop branch (is there a ticket already open for this ?):
File "src/sage/rings/number_field/number_field_ideal.py", line 2199, in sage.rings.number_field.number_field_ideal.NumberFieldFractionalIdeal.invertible_residues Failed example: list(K.ideal(8).invertible_residues())[:5] Expected: [1, a  1, 3*a, 2*a + 3, a  1] Got: [1, a + 2, 3*a + 3, 2*a + 3, a] ********************************************************************** 1 item had failures: 1 of 17 in sage.rings.number_field.number_field_ideal.NumberFieldFractionalIdeal.invertible_residues [685 tests, 1 failure, 6.16 s]
Note that I tested this on version 9.4.beta4.
comment:4 Changed 7 months ago by
 Commit changed from 8ad70e6c63285b65b57b257c7f77d3a404173ad7 to 28c1e2a41633178a4c62d423c999c948a920a26c
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. Last 10 new commits:
34a233d  pull in 32117

cfe869b  modulo n better be for n > 0

3752b9c  some more remaining tests

e6a697a  truely ignore ignored bounds for ZZ.random_element

36c7c58  pull 32124

be3e05f  29979: a few more fixes

5f0f5dc  29979: mark failing tests as not tested

bc1e14a  29979: minor tweaks of doctests

5330692  minor style error

28c1e2a  31414: validate input arguments of elimination_ideal

comment:5 in reply to: ↑ 3 Changed 7 months ago by
 Dependencies changed from #31367 to #29979
 Status changed from needs_review to positive_review
Thank you for the review. To avoid a merge conflict, I have rebased onto #29979. Only the last commit is relevant to this ticket.
Replying to ghDavidAyotte:
I ran the doctest and everything passed except for an unrelated test in src/sage/rings/number_field/number_field_ideal.py which also fail on the develop branch (is there a ticket already open for this ?):
I cannot reproduce it on my end, but there have been discussions about this problem in 31443#comment:119 and 30801#comment:171. Apparently, it depends on whether Pari comes from the system or not. I could not find a ticket specifically for this problem, though maybe I have missed it.
comment:6 Changed 6 months ago by
 Milestone changed from sage9.4 to sage9.5
comment:7 Changed 4 months ago by
 Branch changed from u/ghmwageringel/31414 to 28c1e2a41633178a4c62d423c999c948a920a26c
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
31367: fix elimination in case of quotient rings
31414: validate input arguments of elimination_ideal