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

Python 3: sorting issue causing failure in koszul_complex.py #25978

Closed
jhpalmieri opened this issue Jul 30, 2018 · 10 comments
Closed

Python 3: sorting issue causing failure in koszul_complex.py #25978

jhpalmieri opened this issue Jul 30, 2018 · 10 comments

Comments

@jhpalmieri
Copy link
Member

A sorting issue (TypeError: '>' not supported between instances of 'NoneType' and 'int') in multi_polynomial_ring_base.pyx causes a doctest failure in koszul_complex.py in Python 3.

CC: @fchapoton @tscrim

Component: python3

Author: John Palmieri

Branch/Commit: 4a6ff4e

Reviewer: Travis Scrimshaw

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

@jhpalmieri jhpalmieri added this to the sage-8.4 milestone Jul 30, 2018
@jhpalmieri
Copy link
Member Author

Branch: u/jhpalmieri/koszul-multi-polynomial

@jhpalmieri
Copy link
Member Author

Commit: d8f9668

@jhpalmieri
Copy link
Member Author

comment:2

The issue here is that terms can be None while total is an integer, and then Python 3 objects to if terms > total:.


New commits:

d8f9668trac 25978: don't try to compare None with an int

@tscrim
Copy link
Collaborator

tscrim commented Jul 30, 2018

comment:4

I think it is good to put a comment in there explaining why this test is the way it is.

Also, I am presuming total >= 0 (otherwise, there would be a bug when terms = 0 and total < 0).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2018

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

4a6ff4etrac 25978: add comment explaining the change.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2018

Changed commit from d8f9668 to 4a6ff4e

@jhpalmieri
Copy link
Member Author

comment:6

Replying to @tscrim:

I think it is good to put a comment in there explaining why this test is the way it is.

Sounds good.

Also, I am presuming total >= 0 (otherwise, there would be a bug when terms = 0 and total < 0).

total is defined by counts, total = self._precomp_counts(n, degree), and according to the documentation, total is a sum of cardinalities, so it had better be nonnegative.

@tscrim
Copy link
Collaborator

tscrim commented Jul 30, 2018

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jul 30, 2018

comment:7

Thanks.

@vbraun
Copy link
Member

vbraun commented Sep 1, 2018

Changed branch from u/jhpalmieri/koszul-multi-polynomial to 4a6ff4e

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

3 participants