#18409 closed defect (fixed)
Dynatomic polynomial bug for fractional coefficients
Reported by:  gjorgenson  Owned by:  bhutz 

Priority:  minor  Milestone:  sage6.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^27/4*y^2,y^2]) F = f.dynatomic_polynomial(1) print F.parent()
Change History (17)
comment:1 Changed 5 years ago by
 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 5 years ago by
 Cc bhutz removed
 Commit set to e777520afbd36613e52df9ed04144efc5cb4d0c6
 Owner changed from (none) to bhutz
 Type changed from PLEASE CHANGE to defect
comment:3 Changed 5 years ago by
 Status changed from new to needs_review
comment:4 Changed 5 years ago by
Hello,
Just curiosity, why maxima is involved in this division?
Also, could you add doctests corresponding to the fixed issues?
Vincent
comment:5 Changed 5 years ago by
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 5 years ago by
 Status changed from needs_review to needs_work
comment:7 Changed 5 years ago by
 if (isinstance(period, (list, tuple)) is False): + if not isinstance(period, (list, tuple)):
comment:8 Changed 5 years ago by
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 followup: ↓ 10 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
 Commit changed from e777520afbd36613e52df9ed04144efc5cb4d0c6 to ac037e2e778d739335216429a5a1fb92ab71729f
Branch pushed to git repo; I updated commit sha1. New commits:
ac037e2  18409: fix suggestions from review

comment:12 Changed 5 years ago by
 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 5 years ago by
 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:
ef769e2  18409: further cleanup

comment:14 followup: ↓ 15 Changed 5 years ago by
Those changes look fine, although I find it a little weird to cleanup 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 'needsinfo' on this ticket?
comment:15 in reply to: ↑ 14 Changed 5 years ago by
 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 'needsinfo' on this ticket?
'needsinfo' is because I had a pending question.
Vincent
comment:16 Changed 5 years ago by
 Branch changed from public/18409 to ef769e235d26291f297b88a18b5f438698186768
 Resolution set to fixed
 Status changed from positive_review to closed
comment:17 Changed 5 years ago by
 Commit ef769e235d26291f297b88a18b5f438698186768 deleted
 Milestone changed from sage6.7 to sage6.8
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:
18409: fix output ring for projective dynatomic polynomial