Bug in global_height function

[sagetrac-dkrumm]

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 attachment: trac_11758_global_height.2.patch

CC: @JohnCremona @koffie

Component: number theory

Keywords: global height

Author: David Krumm, Maarten Derickx

Reviewer: Frithjof Schulze

Merged: sage-4.8.alpha5

Issue created by migration from https://trac.sagemath.org/ticket/11758

[JohnCremona]

comment:3

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?

[sagetrac-dkrumm]

comment:4

Replying to @JohnCremona:

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?

[JohnCremona]

comment:5

Replying to @sagetrac-dkrumm:

Replying to @JohnCremona:

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.

[sagetrac-dkrumm]

comment:6

Replying to @JohnCremona:

Replying to @sagetrac-dkrumm:

Replying to @JohnCremona:

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.

[koffie]

comment:7

Replying to @sagetrac-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.

[sagetrac-dkrumm]

comment:8

Replying to @koffie:

Replying to @sagetrac-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?

[JohnCremona]

comment:9

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.

[koffie]

comment:10

I just added the doctest so that the patch is ready for review.

[koffie]

Author: David Krumm, Maarten Derickx

[jdemeyer]

comment:11

Replying to @koffie:

I just added the doctest so that the patch is ready for review.

You need to put a proper commit message in the patch

[koffie]

comment:12

Attachment: trac_11758_global_height.patch.gz

Replying to @jdemeyer:

You need to put a proper commit message in the patch

done

[sagetrac-fschulze]

Original patch, with a trivial typo fixed in a docstring.

[sagetrac-fschulze]

comment:13

Attachment: trac_11758_global_height.2.patch.gz

The above patch fixes a trivial typo in Maarten's patch.

Otherwise everything is fine. Also, Magma gives the same result for both doctests.

[koffie]

comment:14

Thanks for reviewing.

If you review something you should set the reviewer field. And if there are multiple patches you should specify wich to apply.

[koffie]

Reviewer: Frithjof Schulze

[jdemeyer]

Merged: sage-4.8.alpha5