Opened 11 years ago
Closed 10 years ago
#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 )
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.
Attachments (2)
Change History (17)
comment:1 Changed 11 years ago by
- Cc cremona added
comment:2 Changed 11 years ago by
- Cc mderickx added
comment:3 follow-up: ↓ 4 Changed 11 years ago by
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 11 years ago by
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 11 years ago by
- 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 11 years ago by
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 11 years ago by
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 11 years ago by
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.8692222406879748488543678846959454765968722137813736080066given 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 11 years ago by
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.
comment:10 follow-up: ↓ 11 Changed 11 years ago by
- Status changed from new to needs_review
I just added the doctest so that the patch is ready for review.
comment:11 in reply to: ↑ 10 ; follow-up: ↓ 12 Changed 11 years ago by
Replying to mderickx:
I just added the doctest so that the patch is ready for review.
You need to put a proper commit message in the patch
Changed 10 years ago by
comment:12 in reply to: ↑ 11 Changed 10 years ago by
comment:13 Changed 10 years ago by
- Status changed from needs_review to positive_review
The above patch fixes a trivial typo in Maarten's patch.
Otherwise everything is fine. Also, Magma gives the same result for both doctests.
comment:14 Changed 10 years ago by
- Description modified (diff)
- Reviewers set to Frithjof Schulze
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.
comment:15 Changed 10 years ago by
- Merged in set to sage-4.8.alpha5
- Resolution set to fixed
- Status changed from positive_review to closed
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?