Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18409 closed defect (fixed)

Dynatomic polynomial bug for fractional coefficients

Reported by: gjorgenson Owned by: bhutz
Priority: minor Milestone: sage-6.8
Component: algebraic geometry Keywords:
Cc: Merged in:
Authors: Ben Hutz Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: ef769e2 (Commits) Commit:
Dependencies: Stopgaps:

Description

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()

Change History (17)

comment:1 Changed 3 years ago by bhutz

  • Branch set to u/bhutz/ticket/18409
  • Created changed from 05/12/15 16:26:42 to 05/12/15 16:26:42
  • Modified changed from 05/12/15 16:26:42 to 05/12/15 16:26:42

comment:2 Changed 3 years ago by bhutz

  • Authors set to Ben Hutz
  • Cc bhutz removed
  • Commit set to e777520afbd36613e52df9ed04144efc5cb4d0c6
  • Owner changed from (none) to bhutz
  • Type changed from PLEASE CHANGE to defect

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:

e77752018409: fix output ring for projective dynatomic polynomial

comment:3 Changed 3 years ago by bhutz

  • Status changed from new to needs_review

comment:4 Changed 3 years ago by vdelecroix

Hello,

Just curiosity, why maxima is involved in this division?

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

Vincent

comment:5 Changed 3 years ago by vdelecroix

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?

comment:6 Changed 3 years ago by vdelecroix

  • Status changed from needs_review to needs_work

comment:7 Changed 3 years ago by vdelecroix

- if (isinstance(period, (list, tuple)) is False):
+ if not isinstance(period, (list, tuple)):
Last edited 3 years ago by vdelecroix (previous) (diff)

comment:8 Changed 3 years ago by vdelecroix

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.

comment:9 follow-up: Changed 3 years ago by bhutz

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.

comment:10 in reply to: ↑ 9 Changed 3 years ago by vdelecroix

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

comment:11 Changed 3 years ago by git

  • Commit changed from e777520afbd36613e52df9ed04144efc5cb4d0c6 to ac037e2e778d739335216429a5a1fb92ab71729f

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

ac037e218409: fix suggestions from review

comment:12 Changed 3 years ago by bhutz

  • Status changed from needs_work to needs_review

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.

comment:13 Changed 3 years ago by vdelecroix

  • Branch changed from u/bhutz/ticket/18409 to public/18409
  • Commit changed from ac037e2e778d739335216429a5a1fb92ab71729f to ef769e235d26291f297b88a18b5f438698186768
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_info

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:

ef769e218409: further cleanup

comment:14 follow-up: Changed 3 years ago by bhutz

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?

comment:15 in reply to: ↑ 14 Changed 3 years ago by vdelecroix

  • Status changed from needs_info to positive_review

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

comment:16 Changed 3 years ago by vbraun

  • Branch changed from public/18409 to ef769e235d26291f297b88a18b5f438698186768
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:17 Changed 3 years ago by bhutz

  • Commit ef769e235d26291f297b88a18b5f438698186768 deleted
  • Milestone changed from sage-6.7 to sage-6.8
Note: See TracTickets for help on using tickets.