#21100 closed defect (wontfix)

division error in normalize_coordinates

Reported by: bhutz Owned by:
Priority: minor Milestone: sage-duplicate/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 vdelecroix)

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)

flatten.py (2.7 KB) - added by vdelecroix 14 months ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 14 months ago by bhutz

  • Branch set to u/bhutz/division_error_in_normalize_coordinates

comment:2 Changed 14 months ago by bhutz

  • Authors set to Ben Hutz
  • Commit set to 4f9a38ce1c4871cee3c34caf012842356c09bf60
  • Status changed from new to needs_review

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:

4f9a38c21100: fix bug in normalize_coordinates

comment:3 Changed 14 months ago by vdelecroix

  • 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 14 months ago by bhutz

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 stand-alone singular.

comment:5 Changed 14 months ago by vdelecroix

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 14 months ago by git

  • Commit changed from 4f9a38ce1c4871cee3c34caf012842356c09bf60 to 4ba0116b5d85f5511b6eb1ebe8a96a7e55405cf1

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

4ba011621100: add method parameter

comment:7 Changed 14 months ago by bhutz

  • Status changed from needs_work to needs_review

comment:8 Changed 14 months ago by git

  • Commit changed from 4ba0116b5d85f5511b6eb1ebe8a96a7e55405cf1 to 88f76f9966e0df58b539a12205f4fb221972796a

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

88f76f921100: minor corrections

comment:9 Changed 14 months ago by vdelecroix

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 14 months ago by vdelecroix

actually three times!

comment:11 Changed 14 months ago by vdelecroix

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 14 months ago by vdelecroix

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 14 months ago by vdelecroix

comment:13 follow-up: Changed 14 months ago by 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?

comment:14 in reply to: ↑ 13 Changed 14 months ago by vdelecroix

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 14 months ago by bhutz

flattening is now #21106

comment:16 Changed 14 months ago by bhutz

This ticket is now superseded by #21108.

comment:17 Changed 14 months ago by vdelecroix

Not necessarily. In dynatomic_polynomial there are still problematic rings, namely

  • p-adic ring
  • fraction fields

Will they appear in normalize_coordinates?

comment:18 Changed 14 months ago by bhutz

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 14 months ago by bhutz

in further testing, it looks like the _maxima_ still is needed in dynatomic_polynomial

comment:20 Changed 14 months ago by vdelecroix

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 14 months ago by bhutz

  • Milestone changed from sage-7.3 to sage-duplicate/invalid/wontfix

comment:22 Changed 14 months ago by vdelecroix

  • Authors Ben Hutz deleted
  • 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 13 months ago by embray

  • Resolution set to wontfix
  • Status changed from positive_review to closed

Determined to be invalid/duplicate/wontfix (closing as "wontfix" as a catch-all resolution).

Note: See TracTickets for help on using tickets.