Opened 7 years ago

Closed 6 years ago

#17287 closed enhancement (fixed)

K.is_subring(K) not implemented for some fields K

Reported by: mjo Owned by: tmonteil
Priority: major Milestone: sage-6.4
Component: algebra Keywords: beginner
Cc: Merged in:
Authors: Akshay Ajagekar Reviewers: Michael Orlitzky, Thierry Monteil
Report Upstream: N/A Work issues:
Branch: 5d4c880 (Commits, GitHub, GitLab) Commit: 5d4c880067ee7900dbdae6da8207c1339cf6f47d
Dependencies: Stopgaps:

Status badges

Description

This seems like it should be an easy special case if self == foo right before we raise the NotImplementedError.

A few examples:

sage: QQbar.is_subring(QQbar)
...
NotImplementedError
sage: RR.is_subring(RR)
...
NotImplementedError
sage: CC.is_subring(CC)
...
NotImplementedError
sage: K.<a> = NumberField(x^3-x+1/10)
sage: K.is_ring()
True
sage: K.is_subring(K)
...
NotImplementedError
sage: R.<x> = RR[]
sage: R
Univariate Polynomial Ring in x over Real Field with 53 bits of precision
sage: R.is_ring()
True
sage: R.is_subring(R)
...
NotImplementedError

Change History (14)

comment:1 Changed 6 years ago by mjo

  • Keywords beginner added

comment:2 Changed 6 years ago by tmonteil

  • Owner changed from (none) to tmonteil

Let me own this ticket for the next 10 days, i plan to use this for a formation about how to contribute to Sage.

comment:3 Changed 6 years ago by ajagekar.akshay

  • Branch set to u/ajagekar.akshay/Trac17287

comment:4 Changed 6 years ago by ajagekar.akshay

  • Authors set to Akshay Ajagekar
  • Commit set to 2cb2f649aff4ff4b8b75e14c1beb0dc897dd91f3
  • Status changed from new to needs_review

New commits:

2cb2f64Trac #17287: K.is_subring(K) not implemented for some fields K

comment:5 Changed 6 years ago by mjo

  • Reviewers set to Michael Orlitzky

So far so good! Whenever we fix a bug in sage, we like to add some tests to make sure that the bug is really fixed and won't come back again. In the description of this ticket, I found a few examples that used to fail but which now work. Can you add tests for those?

You can see what a test looks like by looking at the commented code right above your fix:

EXAMPLES::

    sage: ZZ.is_subring(QQ)
    True
    sage: ZZ.is_subring(GF(19))
    False

I would suggest adding a new section called "TESTS" and placing the new tests there. The structure of our documentation strings is described in full at http://doc.sagemath.org/html/en/developer/coding_basics.html#documentation-strings. Here's what the first test would look like:

TESTS:

Every ring is a subring of itself, :trac:`17287`::

    sage: QQbar.is_subring(QQbar)
    True

The :trac: thing is a special sort of link to this ticket, described in http://doc.sagemath.org/html/en/developer/sage_manuals.html#chapter-sage-manuals-links. Then you can add the other examples from the description below that. When you're done adding them, don't forget to run sage -b to build the changes (even though you only changed a comment). Then you can run the tests for that file with sage -t. Like,

$ sage -t sage/rings/ring.pyx 
Running doctests with ID 2016-01-31-09-08-05-2b1b6a12.
Git branch: develop
Using --optional=mpir,python2,sage
Doctesting 1 file.
sage -t --warn-long 72.5 sage/rings/ring.pyx
    [387 tests, 4.63 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 4.9 seconds
    cpu time: 3.5 seconds
    cumulative wall time: 4.6 seconds

And it takes forever, but at least once, someone should run make ptestlong to test the entire sage library with the change in place.

comment:6 Changed 6 years ago by tmonteil

  • Reviewers changed from Michael Orlitzky to Michael Orlitzky, Thierry Monteil
  • Status changed from needs_review to needs_work

Great that some newcomer work on it! However, my impression is that this test should be for trivial cases. My fear in using equality is that this might take a while if the equality is hard to decide. Here i guess we want a fast shortcut for trivial cases, that is when self is other (which should solve the present use cases since most rings have unique representation).

comment:7 Changed 6 years ago by git

  • Commit changed from 2cb2f649aff4ff4b8b75e14c1beb0dc897dd91f3 to 9c4d682482208d62144a6ffacf97786e214fcc44

Branch pushed to git repo; I updated commit sha1. New commits:

9c4d682Trac #17287: K.is_subring(K) not implemented for some fields K

comment:8 Changed 6 years ago by ajagekar.akshay

According to suggestions I have made some changes and also added some tests for examples that previously failed.

comment:9 Changed 6 years ago by ajagekar.akshay

  • Status changed from needs_work to needs_review

comment:10 Changed 6 years ago by mjo

Looks good, thanks! I have one more nitpick.

In the docstrings, the double-colons are used to indicate that "the thing after this is a test." So you will see,

EXAMPLES::

    sage: ZZ.is_subring(QQ)
    True

and

TESTS:

Blah blah blah::

    sage: x == x
    True

but in the second case, the "TESTS:" doesn't need two trailing colons.

Finally, these two lines can probably go:

sage: R
Univariate Polynomial Ring in x over Real Field with 53 bits of precision

They don't really test anything useful. I think I included them in the description just to demonstrate that R is a ring.

comment:11 Changed 6 years ago by git

  • Commit changed from 9c4d682482208d62144a6ffacf97786e214fcc44 to 5d4c880067ee7900dbdae6da8207c1339cf6f47d

Branch pushed to git repo; I updated commit sha1. New commits:

5d4c880Trac #17287: K.is_subring(K) not implemented for some fields K

comment:12 Changed 6 years ago by ajagekar.akshay

I corrected the typo and removed unnecessary test.

comment:13 Changed 6 years ago by mjo

  • Status changed from needs_review to positive_review

Everything looks good, thanks!

comment:14 Changed 6 years ago by vbraun

  • Branch changed from u/ajagekar.akshay/Trac17287 to 5d4c880067ee7900dbdae6da8207c1339cf6f47d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.