#24548 closed enhancement (fixed)

py3: various details

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords:
Cc: tscrim, embray, jdemeyer Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 691e9de (Commits) Commit: 691e9de0f194e5928a41c750891b5ebd1d0d7007
Dependencies: Stopgaps:

Description (last modified by chapoton)

  • deprecate some comparison-related methods in free modules

because the cmp keyword will go away in python3

  • various changes about division

Change History (11)

comment:1 Changed 19 months ago by chapoton

  • Description modified (diff)
  • Summary changed from py3: deprecate some comparison-related methods in free modules to py3: various details

comment:2 Changed 19 months ago by chapoton

  • Branch set to u/chapoton/24548
  • Cc tscrim embray jdemeyer added
  • Commit set to 691e9de0f194e5928a41c750891b5ebd1d0d7007
  • Status changed from new to needs_review

New commits:

691e9depy#; various fixes, mostly about division

comment:3 follow-up: Changed 19 months ago by embray

I haven't studied the math in these cases but just to be sure, are you sure these are supposed to be integer division in all these cases? It looks like most of them want integer results, but I'd want to be sure. Have you tested these changes?

comment:4 Changed 19 months ago by chapoton

There could be some slight doubts only in

  • src/sage/algebras/steenrod/steenrod_algebra_bases.py
  • src/sage/modular/btquotients/pautomorphicform.py

but all tests pass in these files. And in these files, the cases of possible doubt are all wrapped with ZZ() or range().

So, this leaves me pretty confident that the math is ok.

comment:5 in reply to: ↑ 3 Changed 19 months ago by jdemeyer

Replying to embray:

I haven't studied the math in these cases but just to be sure, are you sure these are supposed to be integer division in all these cases? It looks like most of them want integer results, but I'd want to be sure. Have you tested these changes?

Also: adding from __future__ import division affects all divisions in that file. In general, I think that adding from __future__ import division is a good idea. But it does mean that one should verify all divisions.

comment:6 Changed 19 months ago by chapoton

green bot

comment:7 Changed 19 months ago by chapoton

ping ?

comment:8 Changed 19 months ago by embray

Apply failed?

comment:9 Changed 19 months ago by chapoton

but TestsPassed on another patchbot..

comment:10 Changed 19 months ago by tscrim

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

LGTM.

comment:11 Changed 19 months ago by vbraun

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