Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not require coefficient's __nonzero__ to be implemented for polynomial printing #23020

Closed
saraedum opened this issue May 17, 2017 · 15 comments

Comments

@saraedum
Copy link
Member

Currently, a polynomial prints all the coefficients that are non-zero.

sage: R.<x> = QQbar[]
sage: R([1,0,1])
x^2 + 1

This of course makes perfect sense but leads to issues when determining whether an element is zero or not is not possible or very hard as is the case with some of the elements introduced in #22956.

There, we essentially have the following situation:

sage: def __nonzero__(self):
....:     raise NotImplementedError()
....: 
sage: sage.rings.qqbar.AlgebraicNumber.__nonzero__ = __nonzero__
sage: R.<x> = QQbar[]
sage: R([1,0,1])
NotImplementedError

This ticket changes the printing to print an element even if __nonzero__ raises an error:

sage: R.<x> = QQbar[]
sage: R([1,0,1])
x^2 + 0*x + 1

CC: @nbruin

Component: commutative algebra

Keywords: sd86.5

Author: Julian Rüth

Branch/Commit: 0734fc5

Reviewer: David Roe

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

@saraedum

This comment has been minimized.

@saraedum
Copy link
Member Author

saraedum commented Jun 7, 2017

@saraedum
Copy link
Member Author

saraedum commented Jun 7, 2017

Changed keywords from none to days86.5

@saraedum
Copy link
Member Author

saraedum commented Jun 7, 2017

New commits:

2e74e54Do not require `__nonzero__` to be implemented for polynomial printing

@saraedum
Copy link
Member Author

saraedum commented Jun 7, 2017

Commit: 2e74e54

@mezzarobba
Copy link
Member

comment:4

I'm not sure if this is relevant, but in many Sage rings, __nonzero__() is understood as “syntactically nonzero” or “not trivially zero”, as opposed to “known not to be zero”. For example, elements of RIF, CIF, RBF, CIF etc. that contain zero but are not point intervals are __nonzero__() . At least some of the generic polynomial/matrix/... code respects this convention. I think it would make sense to have everything in Sage use this definition.

@saraedum
Copy link
Member Author

saraedum commented Jun 8, 2017

comment:5

Thanks for pointing this out. I discussed this with nbruin and we think that regardless, printing of coefficients should be more robust with such exceptions; there is even a point in replacing NotImplementError here with Exception (note that this excludes things such as KeyboardInterrupt.)
So, you are right that I should probably adapt my private code, to implement __nonzero__ differently. But this change makes sense nevertheless.

@saraedum
Copy link
Member Author

saraedum commented Jun 9, 2017

Changed keywords from days86.5 to sd86.5

@roed314
Copy link
Contributor

roed314 commented Jun 10, 2017

comment:8

Looks good to me.

@vbraun
Copy link
Member

vbraun commented Jun 11, 2017

comment:9

Merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2017

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

0734fc5Merge branch 'develop' into t/23020/do_not_require_coefficient_s___nonzero___to_be_implemented_for_polynomial_printing

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2017

Changed commit from 2e74e54 to 0734fc5

@saraedum
Copy link
Member Author

Reviewer: David Roe

@saraedum
Copy link
Member Author

comment:11

Sorry. We do not get any conflicts with the latest beta.


New commits:

0734fc5Merge branch 'develop' into t/23020/do_not_require_coefficient_s___nonzero___to_be_implemented_for_polynomial_printing

New commits:

0734fc5Merge branch 'develop' into t/23020/do_not_require_coefficient_s___nonzero___to_be_implemented_for_polynomial_printing

@vbraun
Copy link
Member

vbraun commented Jun 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants