Opened 4 years ago

Closed 4 years ago

#15373 closed enhancement (fixed)

Implement global_height for Integers

Reported by: paulfili Owned by: paulfili
Priority: minor Milestone: sage-5.13
Component: algebraic geometry Keywords: sage-days55
Cc: Merged in: sage-5.13.beta3
Authors: Paul Fili Reviewers: Adam Towsley
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

QQ(5).global_height() works but ZZ(5).global_height() does not.

Implement global_height for ZZ elements.

Attachments (1)

trac_15373_global_height_ZZ.patch (1.7 KB) - added by paulfili 4 years ago.
Patch that implements global_height for ZZ

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by paulfili

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by paulfili

  • Keywords sage-days55 added; sage-dasy55 removed

comment:3 Changed 4 years ago by paulfili

  • Authors set to Paul Fili

comment:4 Changed 4 years ago by atowsley

  • Status changed from needs_review to positive_review

It passed the doc test and the long test.

Functionality still works.

comment:5 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Please change the commit message to something meaningful and fill in the Reviewer field with your real name.

comment:6 Changed 4 years ago by atowsley

  • Reviewers set to Adam Towsley

comment:7 Changed 4 years ago by paulfili

  • Status changed from needs_work to needs_review

Thanks, I have changed the commit message.

comment:8 Changed 4 years ago by atowsley

  • Status changed from needs_review to positive_review

Everything still works.

comment:9 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to docstring

The indentation of the documentation should be

"""
Documentation here
"""

without the extra indent.

comment:10 Changed 4 years ago by paulfili

  • Status changed from needs_work to needs_review

I have fixed the indentation in the documentation.

comment:11 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

The indentation is still wrong. It should be:

    def global_height(self, prec=None):
        r"""
        Returns the absolute logarithmic height of this rational integer.

        INPUT:

        - ``prec`` (int) -- desired floating point precision (default:
          default RealField precision).

        OUTPUT:

        (real) The absolute logarithmic height of this rational integer.

        ALGORITHM:

        The height of the integer `n` is `\log |n|`.

        EXAMPLES::

            sage: ZZ(5).global_height()
            1.60943791243410
            sage: ZZ(-5).global_height(prec=100)
            1.6094379124341003746007593332
            # This is log(5):
            sage: exp(_)
            5.0000000000000000000000000000
        """

comment:12 Changed 4 years ago by paulfili

  • Status changed from needs_work to needs_review

Thanks for your patience. I've uploaded a new version with the indentation as you specified.

comment:13 Changed 4 years ago by atowsley

I get the following error with the patch...

sage -t --long integer.pyx
**********************************************************************
File "integer.pyx", line 4150, in sage.rings.integer.Integer.global_height
Failed example:
    ZZ(-5).global_height(prec=100)
Expected:
    1.6094379124341003746007593332
    # This is log(5):
Got:
    1.6094379124341003746007593332
**********************************************************************
1 item had failures:
   1 of   4 in sage.rings.integer.Integer.global_height
    [916 tests, 1 failure, 5.69 s]
----------------------------------------------------------------------
sage -t --long integer.pyx  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 6.0 seconds
    cpu time: 4.5 seconds
    cumulative wall time: 5.7 seconds

It's the only error I get, but I don't see why it appears. Since the two values it prints (expected and got) are the same.

Last edited 4 years ago by jdemeyer (previous) (diff)

Changed 4 years ago by paulfili

Patch that implements global_height for ZZ

comment:14 Changed 4 years ago by paulfili

Perhaps the comment on the next line threw it off. I've removed it and put a better example. Should work now.

comment:15 Changed 4 years ago by atowsley

  • Status changed from needs_review to positive_review

Yes, that did it. Everything works now.

comment:16 Changed 4 years ago by jdemeyer

  • Work issues docstring deleted

comment:17 Changed 4 years ago by jdemeyer

  • Merged in set to sage-5.13.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.