Fraction fields should know whether they are finite or not.

[SimonKing]

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​

[SimonKing]

Implement is_finite for fraction fields

[jdemeyer]

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
**********************************************************************

[jdemeyer]

*ping*

[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?

[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.

[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.

[jdemeyer]

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

[SimonKing]

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".

[SimonKing]

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

[SimonKing]

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

[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.

[jdemeyer]

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

[SimonKing]

I merged Jeroens example into my patch. So:

Apply trac11350_fractionfield_is_finite.patch trac11350_docfix.patch

[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.

[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)

[jdemeyer]

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

[SimonKing]

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

[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.

[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.

[SimonKing]

And FWIW: The long doctests pass.

[jhpalmieri]

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.)

[jhpalmieri]

for review only; do not apply

[jhpalmieri]

for review only; do not apply

[robertwb]

I'm fine with the changes.

[SimonKing]

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...

[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".)