Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#22767 closed enhancement (fixed)

py3: some care for division

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

Status badges

Description

correct some of the remaining failures when division uses python3 behaviour

part of #15995

Change History (17)

comment:1 Changed 5 years ago by chapoton

  • Branch set to u/chapoton/22767
  • Commit set to cce963481dbc72cae81fe9c21d1c1be827d16c67
  • Status changed from new to needs_review

New commits:

cc13522py3 add from future import division in one file
c6e4343py3 care for division in rigger configurations
cce9634py: care for division in an elliptic file

comment:2 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_info

Why change int to Integer in various places? I don't think that is the right way to fix things.

comment:3 Changed 5 years ago by chapoton

  • Cc tscrim jmantysalo aapitzsch jdemeyer added

To which of the 3 changed file do you refer ?

I must admit that I have made so that it works, but not tried to understand why.

comment:4 Changed 5 years ago by chapoton

@tscrim: "src/sage/combinat/crystals/virtual_crystal.py" also needs care:

Try adding from __future__ import division there, you will get 4 failures.

comment:5 Changed 5 years ago by tscrim

  • Branch changed from u/chapoton/22767 to public/python3/division_care-22767
  • Commit changed from cce963481dbc72cae81fe9c21d1c1be827d16c67 to 02c2b88accabeb4887916aae42d039e90b93c83e
  • Reviewers set to Travis Scrimshaw

I know the rigged configuration code, so I made the necessary change there. I also fixed the issues in virtual_crystals.py. I have no idea for the other two.


New commits:

69c0491Make virtual_crystal.py python3 compatible.
f945267Merge branch 'u/chapoton/22767' of git://trac.sagemath.org/sage into public/python3/division_care-22767
02c2b88Change for cocharge of rigged configuration elements.

comment:6 Changed 5 years ago by chapoton

may we also undo this change in rigged configurations ?

-        cc = 0
-        rigging_sum = 0
+        cc = ZZ.zero()
+        rigging_sum = ZZ.zero()

comment:7 Changed 5 years ago by tscrim

These are good to have as cc is eventually exposed to the user, and I think could remain an int in certain situations.

comment:8 Changed 5 years ago by chapoton

  • Status changed from needs_info to needs_review

ok, then setting back to needs review

comment:9 Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

need to do something in the elliptic file

comment:10 Changed 5 years ago by git

  • Commit changed from 02c2b88accabeb4887916aae42d039e90b93c83e to 9e38f0e49e9799b024c114cceafd23c1b252db01

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

9250721Merge branch 'public/python3/division_care-22767' in 8.0.b1
9e38f0etrac 22767 one detail in ell_point.py

comment:11 Changed 5 years ago by chapoton

  • Status changed from needs_work to needs_review

New commits:

9250721Merge branch 'public/python3/division_care-22767' in 8.0.b1
9e38f0etrac 22767 one detail in ell_point.py

New commits:

9250721Merge branch 'public/python3/division_care-22767' in 8.0.b1
9e38f0etrac 22767 one detail in ell_point.py

comment:12 Changed 5 years ago by tscrim

Perhaps this is bikeshedding, but I don't understand why this change

-                    r /= v.ramification_index() * v.residue_class_degree()
+                    r = r / (v.ramification_index() * v.residue_class_degree())

You don't need to change it back, but it just feels like it is slightly more complicated code to me.

comment:13 Changed 5 years ago by chapoton

Well, yes, you are right. I was in doubt to what kind of division is performed by /=.

Last edited 5 years ago by chapoton (previous) (diff)

comment:14 Changed 5 years ago by tscrim

If the patchbot comes back green, then you can set a positive review.

comment:15 Changed 5 years ago by chapoton

  • Status changed from needs_review to positive_review

ok,one bot was green

comment:16 Changed 5 years ago by vbraun

  • Branch changed from public/python3/division_care-22767 to 9e38f0e49e9799b024c114cceafd23c1b252db01
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:17 Changed 4 years ago by embray

  • Commit 9e38f0e49e9799b024c114cceafd23c1b252db01 deleted
  • Keywords division added
Note: See TracTickets for help on using tickets.