Opened 10 years ago

Closed 10 years ago

#11350 closed enhancement (fixed)

Fraction fields should know whether they are finite or not.

Reported by: SimonKing Owned by: AlexGhitza
Priority: major Milestone: sage-4.7.2
Component: algebra Keywords: fraction field, is_finite
Cc: Merged in: sage-4.7.2.alpha1
Authors: Simon King Reviewers: Robert Bradshaw, Jeroen Demeyer, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jhpalmieri)

If I am not mistaken, a fraction field of an integral domain R is finite if and only if R is finite. Hence, we should not have a NotImplementedError but the following:

            sage: Frac(QQ['a','b','c']).is_finite()
            False

Apply:

trac_11350-combined.patch

Attachments (6)

trac11350_fractionfield_is_finite.patch (920 bytes) - added by SimonKing 10 years ago.
Implement is_finite for fraction fields
11350_infinite_list.patch (2.5 KB) - added by jdemeyer 10 years ago.
trac11350_docfix.patch (4.2 KB) - added by SimonKing 10 years ago.
Remove a test that was testing for the presence of a bug. Fix another bug and test against its absence
trac_11350-fractionfield-delta.patch (551 bytes) - added by jhpalmieri 10 years ago.
for review only; do not apply
trac_11350-doctest-delta.patch (744 bytes) - added by jhpalmieri 10 years ago.
for review only; do not apply
trac_11350-combined.patch (4.9 KB) - added by jhpalmieri 10 years ago.

Download all attachments as: .zip

Change History (33)

Changed 10 years ago by SimonKing

Implement is_finite for fraction fields

comment:1 Changed 10 years ago by SimonKing

  • Status changed from new to needs_review

comment:2 Changed 10 years ago by robertwb

  • Status changed from needs_review to positive_review

comment:3 Changed 10 years ago by jdemeyer

  • Reviewers set to Robert Bradshaw

comment:4 Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_work
sage -t  -force_lib devel/sage/sage/structure/parent.pyx
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.1.alpha2/devel/sage-main/sage/structure/parent.pyx", line 1099:
    sage: F.is_finite()
Expected:
    Traceback (most recent call last):
    ...
    NotImplementedError
Got:
    False
**********************************************************************

comment:5 follow-up: Changed 10 years ago by jdemeyer

*ping*

comment:6 in reply to: ↑ 5 ; follow-up: Changed 10 years ago by SimonKing

Replying to jdemeyer:

*ping*

Thanks for pinging. I am now trying to find a good replacement for that test.

The problem is: We need an object that has a "list" method (after all, the comments for the test state that F.list() would run forever), and we need one that does not know whether it is finite or not.

So, we can not use fraction fields, since they know whether they are finite. And we can't use, e.g., quotient rings of multivariate polynomial rings, since they do not support iteration.

Do you have any idea what one could use instead?

comment:7 Changed 10 years ago by SimonKing

I just noticed that there is a typo in _list_from_iterator_cached: It says

        try:
            if not self.is_finite():
                raise ValueError('since it is infinite, cannot list %s' % self )
        except AttributeError, NotImplementedError:
            pass

but should of course say

        try:
            if not self.is_finite():
                raise ValueError('since it is infinite, cannot list %s' % self )
        except (AttributeError, NotImplementedError):
            pass

But even if I change this, I have a hard time coming up with an example that illustrates X.list() running forever.

comment:8 in reply to: ↑ 6 Changed 10 years ago by jdemeyer

Replying to SimonKing:

Replying to jdemeyer:

*ping*

Thanks for pinging. I am now trying to find a good replacement for that test.

The problem is: We need an object that has a "list" method (after all, the comments for the test state that F.list() would run forever), and we need one that does not know whether it is finite or not.

I actually don't quite understand the purpose of the current example in structure/parent.pyx because (without the patch) one gets:

sage: R.<t> = QQ[]
sage: F = FractionField(R)
sage: list(F)
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)

/usr/local/src/sage-4.7.1.alpha3/<ipython console> in <module>()

/usr/local/src/sage-4.7.1.alpha3/local/lib/python2.6/site-packages/sage/rings/ring.so in sage.rings.ring.Ring.__iter__ (sage/rings/ring.c:1820)()

NotImplementedError: object does not support iteration
sage: F.list()
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)

/usr/local/src/sage-4.7.1.alpha3/<ipython console> in <module>()

/usr/local/src/sage-4.7.1.alpha3/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.Parent._list_from_iterator_cached (sage/structure/parent.c:7943)()

/usr/local/src/sage-4.7.1.alpha3/local/lib/python2.6/site-packages/sage/rings/ring.so in sage.rings.ring.Ring.is_finite (sage/rings/ring.c:6011)()

NotImplementedError:

So it seems that even the current test doesn't do what is claimed.

comment:9 Changed 10 years ago by jdemeyer

Sorry Simon, that was exactly your point above :-)

comment:10 Changed 10 years ago by SimonKing

  • Status changed from needs_work to needs_review

I have added a second patch.

The present test actually tests for the presence of a bug (namely an object not knowing whether it is finite, although it should know better). In addition, as Jeroen confirms, the test does not do what the comments promise.

Therefore, I removed the test.

In addition, I fixed the bug mentioned above (the missing brackets aroung except AttributeError, NotImplementedError:). I can not give an example of an iterable object that does not know whether it is finite. But still, I am able to show that I fixed the bug, because the resulting error message shows that iteration was attempted.

Moreover, I added a note at a more prominent part of the docstring. It says that, even though X.list() is often better than list(X), it will often be the case that for x in X is better than for x in X.list().

Back to "needs review".

comment:11 Changed 10 years ago by SimonKing

Outch. So, we now have my second patch an Jeroens patch. What shall we do?

Changed 10 years ago by jdemeyer

comment:12 follow-up: Changed 10 years ago by SimonKing

Jeroen, do we really want "#define ENABLE_DEBUG_INTERRUPT 1 ", as in your patch?

comment:13 in reply to: ↑ 12 Changed 10 years ago by jdemeyer

Replying to SimonKing:

Outch. So, we now have my second patch an Jeroens patch. What shall we do?

Read eachothers patches and extract the best out of both of them.

Replying to SimonKing:

Jeroen, do we really want "#define ENABLE_DEBUG_INTERRUPT 1 ", as in your patch?

No, that was a mistake. Fixed now.

comment:14 Changed 10 years ago by jdemeyer

Simon, since you know this stuff better than me, could you make one patch out of our both patches?

comment:15 Changed 10 years ago by SimonKing

  • Description modified (diff)

I merged Jeroens example into my patch. So:

Apply trac11350_fractionfield_is_finite.patch trac11350_docfix.patch

comment:16 Changed 10 years ago by SimonKing

PS: I believe that a full doctest run is required for a new review.

After all, the second patch does change the code, so that some NotImplementedError will be caught where previously it was propagated (although that was a bug). So, it could be that some other tests will fail.

comment:17 Changed 10 years ago by jdemeyer

I think lines 1100--1101 would better be

or not. ::

instead of

or not. 
:: 

(perhaps the latter is correct but it looks unusual to me)

comment:18 Changed 10 years ago by jdemeyer

Also, line 1084: I would say "fixed in #11350" instead of "implemented in #11350".

Changed 10 years ago by SimonKing

Remove a test that was testing for the presence of a bug. Fix another bug and test against its absence

comment:19 Changed 10 years ago by SimonKing

I updated the patch, changing "implemented in #11350" into "fixed in #11350".

However

or not.
::

    sage: ...

is correct. The double colon indicates that a code block follows, but it does not appear as a character.

After applying the patch, just do

sage: R.<t,p> = QQ[]
sage: R.list?

and you'll see that Sage's preprocessing of doc strings does the right thing.

comment:20 Changed 10 years ago by SimonKing

Actually I was not aware that or not. :: is correct as well. I just found:

sage: class A:
....:     """
....:     test it. ::
....:         
....:         sage: bla
....:         blubb
....:         
....:     test the other.
....:     ::
....:         
....:         sage: bla
....:         blubb
....:     """
....:     
sage: A?
Type:		classobj
String Form:	__main__.A
Namespace:	Interactive
File:		/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/IPython/FakeModule.py
Docstring:
       test it.
    
          sage: bla
          blubb
    
       test the other.
    
          sage: bla
          blubb

So, both versions are preprocessed correctly.

comment:21 Changed 10 years ago by SimonKing

And FWIW: The long doctests pass.

comment:22 Changed 10 years ago by jhpalmieri

  • Description modified (diff)
  • Reviewers changed from Robert Bradshaw to Robert Bradshaw, Jeroen Demeyer, John Palmieri

I'm basically happy with the patches here, except for some wording and formatting in the docstrings. I'm attaching a combined version of the two patches, along with "delta" patches for each so you can see what I changed. If you're happy with my changes, please give the ticket a positive review.

(Basically: in trac11350_fractionfield_is_finite.patch, I changed the "NOTE" to an actual ".. note::" markup, and I reworded the note a bit. In the other attachment, I reworded things a bit also. Since the docstring there is not actually in the reference manual, since the method starts with an underscore, I didn't bother putting in the ".. note::" markup. If you feel like doing that, go ahead.)

Changed 10 years ago by jhpalmieri

for review only; do not apply

Changed 10 years ago by jhpalmieri

for review only; do not apply

Changed 10 years ago by jhpalmieri

comment:23 Changed 10 years ago by robertwb

I'm fine with the changes.

comment:24 Changed 10 years ago by SimonKing

  • Status changed from needs_review to positive_review

Fine to me as well - unless there is a difference between .. NOTE:: (as in the combined patch) and .. note:: (which was intended to be used according to John's post above).

So, I take it as a positive review, then, unless someone has a veto because of upper versus lower case note...

comment:25 Changed 10 years ago by jhpalmieri

Both upper and lower case work: they're interchangeable. Someone has even expressed the opinion that we should use use the upper-case ones in the Sage documentation, although I personally don't feel very strongly about it. (See the developer's guide and search for "notes block".)

comment:26 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-4.7.1 to sage-4.7.2

comment:27 Changed 10 years ago by jdemeyer

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