Opened 3 years ago
Closed 3 years ago
#21100 closed defect (wontfix)
division error in normalize_coordinates
Reported by:  bhutz  Owned by:  

Priority:  minor  Milestone:  sageduplicate/invalid/wontfix 
Component:  algebraic geometry  Keywords:  
Cc:  Merged in:  
Authors:  Reviewers:  Ben Hutz  
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Note: This ticket is now superseded by #21108.
When the gcd is a multivariate polynomial, it does not cancel upon scaling
R.<a,b>=QQ[] P.<x,y,z>=ProjectiveSpace(R,2) H=End(P) f=H([a*(x+y)*x^2,b*(x+y)*y^2,(x+y)*z^2]) f.normalize_coordinates()
This division can be done through maxima
Attachments (1)
Change History (24)
comment:1 Changed 3 years ago by
 Branch set to u/bhutz/division_error_in_normalize_coordinates
comment:2 Changed 3 years ago by
 Commit set to 4f9a38ce1c4871cee3c34caf012842356c09bf60
 Status changed from new to needs_review
comment:3 Changed 3 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to needs_work
I would prefer
def normalize_coordinates(self, method=None): if method is None: # document why it is done this way try: self.normalize_coordinates(method='singular') except (TypeError, NotImplementedError): self.normalize_coordinates(method='maxima') elif method == 'singular': # do singular way elif method == 'maxima': # do maxima way else: raise ValueError("method={} not available".format(method))
This way you could have doctests for:
 the singular failure reported in this doctest
 comparison between maxima and singular versions
By the way, why do you catch both TypeError
and NotImplementedError
?
Does it work on last versions of singular?
comment:4 Changed 3 years ago by
Yes, I see the logic in that. I can do it that way.
err...I'll need to check but the NotImplementedError? may not occur in this case. That is a carry over from dynatomic_polynomial where a similar thing is done. I'll look into the 'method' approach for that function too so the singular failure can be tracked.
as for the most recent version of singular. I'm not sure, this fails on sage 7.3.beta9 and whatever singular is in that. I don't have a standalone singular.
comment:5 Changed 3 years ago by
We are far away in Sage. Singular 4.0.2 is available (see #17254 almost 2 years old ticket) and Sage ships 3.1.7.
comment:6 Changed 3 years ago by
 Commit changed from 4f9a38ce1c4871cee3c34caf012842356c09bf60 to 4ba0116b5d85f5511b6eb1ebe8a96a7e55405cf1
Branch pushed to git repo; I updated commit sha1. New commits:
4ba0116  21100: add method parameter

comment:7 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:8 Changed 3 years ago by
 Commit changed from 4ba0116b5d85f5511b6eb1ebe8a96a7e55405cf1 to 88f76f9966e0df58b539a12205f4fb221972796a
Branch pushed to git repo; I updated commit sha1. New commits:
88f76f9  21100: minor corrections

comment:9 Changed 3 years ago by
I did not see, but with what I proposed you might end up computing twice the GCD... don't know if it is a big problem.
comment:10 Changed 3 years ago by
actually three times!
comment:11 Changed 3 years ago by
Another option seems to flatten the polynomial ring in order to make singular happier
sage: R.<a,b> = QQ[] sage: P.<x,y,z> = R[] sage: p0 = a*(x+y)*x^2 sage: g = x + y sage: p0.quo_rem(g) Traceback (most recent call last): ... TypeError: no conversion of this ring to a Singular ring defined sage: Pflat.<x,y,z,a,b> = QQ[] sage: p0 = Pflat(str(p0)) # conversion hack ;) sage: g = Pflat(g) sage: p0.quo_rem(g) (x^2*a, 0)
comment:12 Changed 3 years ago by
We can introduce a method to multivariate polynomial ring to build a "flattening morphism" like QQ[a,b][x,y,z] > QQ[a,b,x,y,z]
(as soon as variable names do not clash).
Changed 3 years ago by
comment:13 followup: ↓ 14 Changed 3 years ago by
Yes, I wasn't very happy with computing the gcd multiple times, but it did track the issue and the gcd is faster than the actual division step, so it wasn't a big performance hit in what we were doing.
I didn't experiment with the flattening code, but that sounds promising. I think it would also be useful for specialization in families.
Do you want to add the flattening functionality to polynomial rings, or have me take your flatten.py and flesh it out?
comment:14 in reply to: ↑ 13 Changed 3 years ago by
Replying to bhutz:
Yes, I wasn't very happy with computing the gcd multiple times, but it did track the issue and the gcd is faster than the actual division step, so it wasn't a big performance hit in what we were doing.
I didn't experiment with the flattening code, but that sounds promising. I think it would also be useful for specialization in families.
Do you want to add the flattening functionality to polynomial rings, or have me take your flatten.py and flesh it out?
I guess the flattening can be done in another ticket. Concerning this one, it can either wait for flattening or it can be closed and refactored later on. If you have time for working on it please go ahead.
comment:15 Changed 3 years ago by
flattening is now #21106
comment:16 Changed 3 years ago by
This ticket is now superseded by #21108.
comment:17 Changed 3 years ago by
Not necessarily. In dynatomic_polynomial
there are still problematic rings, namely
 padic ring
 fraction fields
Will they appear in normalize_coordinates
?
comment:18 Changed 3 years ago by
No, the further problematic rings treated in dynatomic don't have gcds, so it doesn't get that far.
I do think some of the special casing in dynatomic can now also be removed. I was going to do that in a different ticket.
comment:19 Changed 3 years ago by
in further testing, it looks like the _maxima_ still is needed in dynatomic_polynomial
comment:20 Changed 3 years ago by
What do you want to do with this one? If you want to close it you should change the milestone to duplicate/won't fix
.
comment:21 Changed 3 years ago by
 Milestone changed from sage7.3 to sageduplicate/invalid/wontfix
comment:22 Changed 3 years ago by
 Branch u/bhutz/division_error_in_normalize_coordinates deleted
 Commit 88f76f9966e0df58b539a12205f4fb221972796a deleted
 Description modified (diff)
 Reviewers changed from Vincent Delecroix to Ben Hutz
 Status changed from needs_review to positive_review
comment:23 Changed 3 years ago by
 Resolution set to wontfix
 Status changed from positive_review to closed
Determined to be invalid/duplicate/wontfix (closing as "wontfix" as a catchall resolution).
This fixes the found bug. I haven't found any additional examples that we should be able to compute that we cannot. Searching for some additional difficult examples would be good.
New commits:
21100: fix bug in normalize_coordinates