Context Navigation

← Previous Ticket
Next Ticket →

#11758 closed defect (fixed)

Bug in global_height function

Reported by:	dkrumm	Owned by:	cremona
Priority:	major	Milestone:	sage-4.8
Component:	number theory	Keywords:	global height
Cc:	cremona, mderickx	Merged in:	sage-4.8.alpha5
Authors:	David Krumm, Maarten Derickx	Reviewers:	Frithjof Schulze
Report Upstream:	N/A	Work issues:
Branch:		Commit:
Dependencies:		Stopgaps:

Description (last modified by mderickx)

The global_height function for elements of number fields gives incorrect results. Here is an example:

sage: K.<s> = QuadraticField(2)
sage: s.global_height()
0.346573590279973
sage: (1/s).global_height()
0.693147180559945

This is incorrect since s and 1/s should have the same height. I'm running Sage 4.7 on Mac OS X 10.6.8.

I believe the reason for the error is explained in the author's comments in the code for this function:

"The absolute logarithmic height of this number field element; that is, the sum of the local heights at all finite and infinite places, with the contributions from the infinite places scaled by the degree to make the result independent of the parent field."

However, it is both the arch. and non-arch. contributions that need to be scaled by the degree.

apply trac_11758_global_height.2.patch

Attachments (2)

trac_11758_global_height.patch (2.3 KB) - added by mderickx 9 years ago.
trac_11758_global_height.2.patch (2.3 KB) - added by fschulze 9 years ago.: Original patch, with a trivial typo fixed in a docstring.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by dkrumm

Cc cremona added

comment:2 Changed 9 years ago by mderickx

Cc mderickx added

comment:3 follow-up: ↓ 4 Changed 9 years ago by cremona

This looks like a valid bug to me (the author, sorry). Would you (dkrumm) like to make a patch fixing it, with your example as a doctest?

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 9 years ago by dkrumm

Replying to cremona:

This looks like a valid bug to me (the author, sorry). Would you (dkrumm) like to make a patch fixing it, with your example as a doctest?

Yes, I'd like to do that. I'm trying to figure out how to make a patch (I'm completely new to Sage development), but it should not take too long. In a related question, I have my own height function that I use instead of this one, in which the result is guaranteed to be correct within any given accuracy, and I think the current height in Sage does not (the precision in the input refers not to the accuracy of the output, but to the accuracy used in computing the embeddings of the number field). Do you think it would be good to try to include this height function into Sage?

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 9 years ago by cremona

Owner changed from somebody to cremona

Replying to dkrumm:

Replying to cremona:

This looks like a valid bug to me (the author, sorry). Would you (dkrumm) like to make a patch fixing it, with your example as a doctest?

Yes, I'd like to do that. I'm trying to figure out how to make a patch (I'm completely new to Sage development), but it should not take too long. In a related question, I have my own height function that I use instead of this one, in which the result is guaranteed to be correct within any given accuracy, and I think the current height in Sage does not (the precision in the input refers not to the accuracy of the output, but to the accuracy used in computing the embeddings of the number field). Do you think it would be good to try to include this height function into Sage?

Definitely. Do it! But on this ticket, limit the patch to fixing the bug (with doctest), and then create a new ticket for the better function, with a corss-reference from here.

comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 9 years ago by dkrumm

Replying to cremona:

Replying to dkrumm:

Replying to cremona:

This looks like a valid bug to me (the author, sorry). Would you (dkrumm) like to make a patch fixing it, with your example as a doctest?

Yes, I'd like to do that. I'm trying to figure out how to make a patch (I'm completely new to Sage development), but it should not take too long. In a related question, I have my own height function that I use instead of this one, in which the result is guaranteed to be correct within any given accuracy, and I think the current height in Sage does not (the precision in the input refers not to the accuracy of the output, but to the accuracy used in computing the embeddings of the number field). Do you think it would be good to try to include this height function into Sage?

Definitely. Do it! But on this ticket, limit the patch to fixing the bug (with doctest), and then create a new ticket for the better function, with a corss-reference from here.

I hope I've created the patch correctly. Sorry, I don't know what doctest is.

comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 9 years ago by mderickx

Replying to dkrumm:

I hope I've created the patch correctly. Sorry, I don't know what doctest is.

A doctest is some test contained in the documentation of a function. Just do:

sage: K.<s> = QuadraticField(2)
sage: s.global_height?

You should now see documentation on how to use the global height. The examples like:

          sage: R.<x> = QQ[]
          sage: K.<a> = NumberField(x^4+3*x^2-17)
          sage: b = a/2
          sage: b.global_height()
          2.869222240687...
          sage: b.global_height(prec=200)
          2.8692222406879748488543678846959454765968722137813736080066

given there are called doctests since they are used for automated testing of the sage library (see http://www.sagemath.org/doc/developer/doctesting.html). All those examples are tested and should produce the shown output before a patch is accepted. In this case it means that the doctests have to change since the answers show are not correct. What john asked is if you could also add your example wich you posted in the description as a test (ofcourse with the new and imporved answers.

comment:8 in reply to: ↑ 7 Changed 9 years ago by dkrumm

Replying to mderickx:

Replying to dkrumm:

I hope I've created the patch correctly. Sorry, I don't know what doctest is.

A doctest is some test contained in the documentation of a function. Just do:
sage: K.<s> = QuadraticField(2)
sage: s.global_height?
You should now see documentation on how to use the global height. The examples like:
          sage: R.<x> = QQ[]
          sage: K.<a> = NumberField(x^4+3*x^2-17)
          sage: b = a/2
          sage: b.global_height()
          2.869222240687...
          sage: b.global_height(prec=200)
          2.8692222406879748488543678846959454765968722137813736080066
given there are called doctests since they are used for automated testing of the sage library (see http://www.sagemath.org/doc/developer/doctesting.html). All those examples are tested and should produce the shown output before a patch is accepted. In this case it means that the doctests have to change since the answers show are not correct. What john asked is if you could also add your example wich you posted in the description as a test (ofcourse with the new and imporved answers.

Ok, thanks for the explanation. Unfortunately, I can't put in those examples yet, because my sage library won't build. So even though I've made the changes in the code, I can't get sage to recognize them. I have this problem on my personal computer, and also on the ones in my department.. do you think this problem with Lion and Xcode will be fixed any time soon?

comment:9 Changed 9 years ago by cremona

If you have problems building Sage ask questions on sage-devel, not here.

Read the developers' guide for instructions on how to write doctests and make patches, etc. And I suggest that you don't post a patch until you have tested it which you obviously cannot do until you have your own development build up and running.