Opened 3 years ago

Closed 3 years ago

#25978 closed defect (fixed)

Python 3: sorting issue causing failure in koszul_complex.py

Reported by: jhpalmieri Owned by:
Priority: minor Milestone: sage-8.4
Component: python3 Keywords:
Cc: chapoton, tscrim Merged in:
Authors: John Palmieri Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 4a6ff4e (Commits, GitHub, GitLab) Commit: 4a6ff4e6c396231908bd58758e46a9f0bb24dae2
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (8)

comment:1 Changed 3 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/koszul-multi-polynomial

comment:2 Changed 3 years ago by jhpalmieri

  • Commit set to d8f9668efec431c33593f7490a78e22f6a8ebfd9

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

comment:3 Changed 3 years ago by jhpalmieri

  • Status changed from new to needs_review

comment:4 follow-up: Changed 3 years ago by tscrim

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

comment:5 Changed 3 years ago by git

  • Commit changed from d8f9668efec431c33593f7490a78e22f6a8ebfd9 to 4a6ff4e6c396231908bd58758e46a9f0bb24dae2

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

​4a6ff4etrac 25978: add comment explaining the change.

comment:6 in reply to: ↑ 4 Changed 3 years ago by jhpalmieri

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.

comment:7 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thanks.

comment:8 Changed 3 years ago by vbraun

  • Branch changed from u/jhpalmieri/koszul-multi-polynomial to 4a6ff4e6c396231908bd58758e46a9f0bb24dae2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.