Opened 5 years ago

Closed 5 years ago

#22232 closed enhancement (fixed)

py3 cleanup of symmetric_reduction.pyx

Reported by: chapoton Owned by:
Priority: major Milestone: sage-7.6
Component: python3 Keywords:
Cc: tscrim, jdemeyer, aapitzsch Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: e88d4c7 (Commits, GitHub, GitLab) Commit: e88d4c72615aa6abe694f9ea7cf3e520cfcbd839
Dependencies: Stopgaps:

Status badges

Description

remove all call to cmp()

plus some pep8 changes

Change History (8)

comment:1 Changed 5 years ago by chapoton

  • Branch set to u/chapoton/22232
  • Cc tscrim jdemeyer aapitzsch added
  • Commit set to a93e1cef43252ddc7d1bfa103675a9fadbace599
  • Status changed from new to needs_review

New commits:

a93e1cepython3 and pep8 cleanup of symmetric_reduction.pyx

comment:2 Changed 5 years ago by tscrim

I disagree with this change:

-        self._lm = [R(x) for x in self._lm] # I have no idea why -- but it seems needed.
+        self._lm = [R(x) for x in self._lm]
+        # I have no idea why -- but it seems needed

as it breaks the association of the comment with the line in question.

I also (still) disagree with returning NotImplemented when the classes do not match.

Could we also remove some of the useless parentheses, such as while (p._p != 0):?

comment:3 Changed 5 years ago by git

  • Commit changed from a93e1cef43252ddc7d1bfa103675a9fadbace599 to acffce79a8374dbc51bfc245c4d6fe98fb69677f

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

acffce7trac 22232 some details

comment:4 Changed 5 years ago by chapoton

Just done the trivial changes.

About the NotImplemented?, why do you disagree ? Python will know what to do with this answer, in principle. This is not an Error, but probably behaves more somehow like an "unknown" value.

You may have already explained, but I still see no other reasonable possibility.

comment:5 Changed 5 years ago by tscrim

We do know that they are not equal or comparable when the types do not match. It's not like there is ambiguity with, e.g., symbolics.

comment:6 Changed 5 years ago by git

  • Commit changed from acffce79a8374dbc51bfc245c4d6fe98fb69677f to e88d4c72615aa6abe694f9ea7cf3e520cfcbd839

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

e88d4c7trac 22232 detail

comment:7 Changed 5 years ago by tscrim

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

That is acceptable to me.

comment:8 Changed 5 years ago by vbraun

  • Branch changed from u/chapoton/22232 to e88d4c72615aa6abe694f9ea7cf3e520cfcbd839
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.