Opened 7 years ago

Closed 7 years ago

#1107 closed enhancement (fixed)

[with patch, with positive review] add minkowski bound function for number fields

Reported by: was Owned by: was
Priority: minor Milestone: sage-2.8.15
Component: number theory Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Add computation of Minkowski bound to number fields (very simple).

Attachments (1)

minkowski.patch (3.4 KB) - added by was 7 years ago.
This is rebased against 2.8.15

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by mabshoff

  • Milestone changed from sage-2.9 to sage-2.8.13

comment:2 Changed 7 years ago by robertwb

The patch is good.

comment:3 Changed 7 years ago by mabshoff

The patch no longer applies cleanly:

mabshoff@sage:/tmp/Work-mabshoff/release-cycles/sage-2.8.13.alpha0/devel/sage$ hg import minkowski.patch
applying minkowski.patch
patching file sage/rings/rational_field.py
Hunk #1 succeeded at 298 with fuzz 2 (offset 23 lines).
Hunk #2 FAILED at 362
Hunk #3 FAILED at 370
Hunk #4 FAILED at 378
3 out of 4 hunks FAILED -- saving rejects to file sage/rings/rational_field.py.rej
abort: patch failed to apply

Cheers,

Michaell

comment:4 Changed 7 years ago by cwitty

  • Summary changed from [with patch] add minkowski bound function for number fields to [with broken patch] add minkowski bound function for number fields

comment:5 Changed 7 years ago by was

cwitty: williamstein, did you notice mabshoff's comment on your #1107 patch?  Evidently it no longer applies.
[9:13pm] williamstein: Thanks.  
[9:16pm] williamstein: actually it's fine -- the one hunk that doesn't get applied with 1107 doesn't apply because it is already applied in the current sage.
[9:16pm] williamstein: So it's OK.  Just ignore the one hunk that fails. 

comment:6 Changed 7 years ago by was

[9:17pm] cwitty: The three hunks that don't get applied, you mean?  (Judging from mabshoff's comment.)
[9:20pm] williamstein: Yes, that's what I meant.  Thanks.

comment:7 Changed 7 years ago by cwitty

  • Summary changed from [with broken patch] add minkowski bound function for number fields to [with patch] add minkowski bound function for number fields

Changed 7 years ago by was

This is rebased against 2.8.15

comment:8 Changed 7 years ago by was

OK, I rebased it so I get credit :-)

comment:9 Changed 7 years ago by robertwb

  • Summary changed from [with patch] add minkowski bound function for number fields to [with patch, with positive review] add minkowski bound function for number fields

Looks good to me.

comment:10 Changed 7 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in 2.8.15.alpha0.

Note: See TracTickets for help on using tickets.