Ticket #1107 (closed enhancement: fixed)

Opened 6 years ago

Last modified 5 years ago

[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: Work issues:
Report Upstream: Reviewers:
Authors: Merged in:
Dependencies: Stopgaps:

Description

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

Attachments

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

Change History

comment:1 Changed 6 years ago by mabshoff

  • Milestone changed from sage-2.9 to sage-2.8.13

comment:2 Changed 6 years ago by robertwb

The patch is good.

comment:3 Changed 6 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 5 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 5 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 5 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 5 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 5 years ago by was

This is rebased against 2.8.15

comment:8 Changed 5 years ago by was

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

comment:9 Changed 5 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 5 years ago by mabshoff

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

Merged in 2.8.15.alpha0.

Note: See TracTickets for help on using tickets.