Dynatomic polynomial bug for fractional coefficients

[sagetrac-gjorgenson]

Dynatomic polynomial returns a symbolic ring element when passed a map with a constant denominator != 1:

P.<x,y> = ProjectiveSpace(QQ,1)
H = Hom(P,P)
f = H([x^2-7/4*y^2,y^2])
F = f.dynatomic_polynomial(1)
print F.parent()

Component: algebraic geometry

Author: Ben Hutz

Branch: ef769e2

Reviewer: Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/18409

[bhutz]

Branch: u/bhutz/ticket/18409

[bhutz]

comment:2

I fixed a number of cases where Symbolic Ring was being returned by changing how the conversion was being done and not doing the (unnecessary) division in the [0,1] case.

There are still some obscure cases where we can't get back from the Symbolic Ring. Such as certain maps where the coordinate ring is a function field over QQbar.

---

New commits:

e777520

18409: fix output ring for projective dynatomic polynomial

[bhutz]

Commit: e777520

[bhutz]

Author: Ben Hutz

[videlec]

comment:4

Hello,

Just curiosity, why maxima is involved in this division?

Also, could you add doctests corresponding to the fixed issues?

Vincent

[videlec]

comment:5

In the very same same functions, both the if block and the if code have these definition

        x = self.domain().gen(0)
        y = self.domain().gen(1)
        F = self._polys
        f = F
        PHI = 1 

could you move it before?

[videlec]

comment:7

- if (isinstance(period, (list, tuple)) is False):
+ if not isinstance(period, (list, tuple)):

[videlec]

comment:8

This is very strange

    if d != n: # avoid extra iteration
...
    if d != period[1]: # avoid extra iteration

You can easily predict when this happen: every time excepted the last run of the loop. But with the above code you test in each run of the loop whether d == period or not. So please, decrease the upper bound of the loop by one and copy the command that has to be executed. It would be cleaner and clearer.

[bhutz]

comment:9

Thanks for the comments Vincent. I can certainly make those code improvements.

For the examples, I wasn't sure about adding them. What is changing here is the parent of the result, not the actual result. In particular, the new version is converting some of the Symoblic Ring cases back to a polynomial. So a new example would need to test the parent of the result, which seemed a little odd.

Maxima was originally involved because there were cases where the quo_rem function was not doing the division; I believe cases like the base ring is a function field or polynomial ring. Maxima is able to do this division.

I'll run some more tests and see if the pass to Maxima is still needed, or if we can avoid that situation completely.

[videlec]

comment:10

Hi Ben,

Replying to @bhutz:

For the examples, I wasn't sure about adding them.

It will be very useful to detect regression. You can add something along

TESTS:

We check that for some delicate polynomial ring, the dynatomic polynomial has
the right parent (see :trac:`18409`)::

    sage: p1 = my_example1
    sage: parent(p1)
    ...

    sage: p2 = my_example2
    sage: parent(p2)
    ...

Though, this one still does not work::

    sage: p3 = my_example3
    sage: parent(p3)
    The Symbolic Ring

What is changing here is the parent of the result, not the actual result. In particular, the new version is converting some of the Symoblic Ring cases back to a polynomial. So a new example would need to test the parent of the result, which seemed a little odd.

If the parent changes, then the result does change! This kind of things is exactly the purpose of the TESTS section. Note that there are examples of that in the Sage library (like checking that the output is a Sage Integer and not an int, or similar things).

Vincent

[sagetrac-git]

Changed commit from e777520 to ac037e2

[sagetrac-git]

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

ac037e2

18409: fix suggestions from review

[bhutz]

comment:12

Those changes are made and I added a comment about Maxima. I had forgotten about the TESTS section. I've added those examples there and improved one of the EXAMPLES.

[videlec]

Changed branch from u/bhutz/ticket/18409 to public/18409

[videlec]

comment:13

Hello,

I did further cleanup (see the new branch public/18409).

In the case the ring is wrong, I would emit a warning to alert the user. See python module warnings. Just put

import warnings
warnings.warn("Be careful, the output from method X is a strange object")

The handling of warnings is not yet fantastic in Sage since this will be only used once... (at the second call nothing happens). But at least the user will be noticed.

Vincent

---

New commits:

ef769e2

18409: further cleanup

[videlec]

Reviewer: Vincent Delecroix

[videlec]

Changed commit from ac037e2 to ef769e2

[bhutz]

comment:14

Those changes look fine, although I find it a little weird to clean-up nth_iterate_map() inside the ticket for dynatomic_polynomial(), but those changes test out fine for me.

I played around with the user warning and it really is not helpful. It is clearly stated in the doc for the function what is happening. I could see an argument for an actual warning block in the docs, but not the message printed to the user. How strongly do you want to see a user warning?

Not sure what 'needs-info' on this ticket?

[videlec]

comment:15

Replying to @bhutz:

I played around with the user warning and it really is not helpful. It is clearly stated in the doc for the function what is happening. I could see an argument for an actual warning block in the docs, but not the message printed to the user. How strongly do you want to see a user warning?

The problem is that some people might not read the whole documentation of the function before using it. This is not very important though.

Not sure what 'needs-info' on this ticket?

'needs-info' is because I had a pending question.

Vincent

[vbraun]

Changed branch from public/18409 to ef769e2

[bhutz]

Changed commit from ef769e2 to none