Opened 2 months ago

Closed 8 weeks ago

#26914 closed enhancement (fixed)

py3: fixing doctests in elliptic_curves/cm.py

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.6
Component: python3 Keywords:
Cc: tscrim, cremona Merged in:
Authors: Frédéric Chapoton Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: 5403ce7 (Commits) Commit: 5403ce7894ea03db25c8fcf23ae156993b54c4cf
Dependencies: Stopgaps:

Description

by more sorting of discriminants

Change History (8)

comment:1 Changed 2 months ago by chapoton

  • Branch set to u/chapoton/26914
  • Commit set to 9d5f2f9ad4b311b8775031624c5fc0b54c124bbf
  • Status changed from new to needs_review

New commits:

9d5f2f9py3: fixes in elliptic_curves/cm.py

comment:2 Changed 2 months ago by cremona

All looks fine apart from this:

-    dlist = sum([v for h, v in iteritems(discriminants_with_bounded_class_number(K.degree(), proof=proof))], [])
+    dlist = sorted(Df for v in discriminants_with_bounded_class_number(K.degree(), proof=proof).values() for Df in v)

whose syntax looks suspicious to me.

comment:3 follow-up: Changed 2 months ago by chapoton

Hmm. Why suspicious ? The "sorted" is the only real change we are making here. If my understanding of the mathematics involved is ok, this should not be a problem.

Or maybe you are troubled by the double for ? This is a perfectly valid python syntax. We can chain the "for", the second one depending on the argument of the first. Here the effect is to flatten a list of lists.

https://www.python.org/dev/peps/pep-0289/

comment:4 in reply to: ↑ 3 Changed 2 months ago by cremona

Replying to chapoton:

Hmm. Why suspicious ? The "sorted" is the only real change we are making here. If my understanding of the mathematics involved is ok, this should not be a problem.

Or maybe you are troubled by the double for ? This is a perfectly valid python syntax. We can chain the "for", the second one depending on the argument of the first. Here the effect is to flatten a list of lists.

https://www.python.org/dev/peps/pep-0289/

Thanks for the lesson! I had never seen a double for so thought it was a mistake (and did not have time to check).

comment:5 Changed 2 months ago by git

  • Commit changed from 9d5f2f9ad4b311b8775031624c5fc0b54c124bbf to 5403ce7894ea03db25c8fcf23ae156993b54c4cf

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

5403ce7trac 26914 fix pyflakes warning

comment:6 Changed 2 months ago by chapoton

green bot, ready for review

comment:7 Changed 2 months ago by cremona

  • Reviewers set to John Cremona
  • Status changed from needs_review to positive_review

comment:8 Changed 8 weeks ago by vbraun

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