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: |
Description (last modified by )
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:
Attachments (6)
Change History (33)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Status changed from needs_review to positive_review
comment:3 Changed 10 years ago by
- Reviewers set to Robert Bradshaw
comment:4 Changed 10 years ago by
- 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: ↓ 6 Changed 10 years ago by
*ping*
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 10 years ago by
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
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
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
Sorry Simon, that was exactly your point above :-)
comment:10 Changed 10 years ago by
- 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
Outch. So, we now have my second patch an Jeroens patch. What shall we do?
Changed 10 years ago by
comment:12 follow-up: ↓ 13 Changed 10 years ago by
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
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
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
- 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
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
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
Changed 10 years ago by
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
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
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
And FWIW: The long doctests pass.
comment:22 Changed 10 years ago by
- 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
comment:23 Changed 10 years ago by
I'm fine with the changes.
comment:24 Changed 10 years ago by
- 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
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
- Milestone changed from sage-4.7.1 to sage-4.7.2
comment:27 Changed 10 years ago by
- Merged in set to sage-4.7.2.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
Implement is_finite for fraction fields