Opened 9 years ago
Last modified 10 months ago
#9970 needs_work defect
Flaky doctest in sage/interfaces/r.py
Reported by: | mpatel | Owned by: | mvngu |
---|---|---|---|
Priority: | critical | Milestone: | sage-6.5 |
Component: | doctest coverage | Keywords: | r |
Cc: | dimpase, kcrisman, nthiery, gh-timokau | Merged in: | |
Authors: | Willem Jan Palenstijn | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This test in src/sage/interfaces/r.py
should be fixed:
sage: os.path.realpath(tmpdir) == sageobj(r.getwd()) # known bug (:trac:`9970`) True
The script
import os import tempfile fail = 0 for i in xrange(1000): tmpdir = tempfile.mkdtemp() r.chdir(tmpdir) if tmpdir not in sageobj(r.getwd()): print tmpdir, r.getwd(), sageobj(r.getwd()) fail += 1 os.rmdir(tmpdir) # clean up print fail
prints
/tmp/tmpb1LEIM [1] "/tmp/tmpb1LEIM" /tmp/tmpbInteger(1)EIM /tmp/tmpZ6LyPm [1] "/tmp/tmpZ6LyPm" /tmp/tmpZInteger(6)yPm /tmp/tmp2LTCse [1] "/tmp/tmp2LTCse" /tmp/tmpInteger(2)TCse /tmp/tmpzoor1L [1] "/tmp/tmpzoor1L" /tmp/tmpzoorInteger(1) /tmp/tmpCl70Lo [1] "/tmp/tmpCl70Lo" /tmp/tmpClInteger(70)o /tmp/tmpN7L_vQ [1] "/tmp/tmpN7L_vQ" /tmp/tmpNInteger(7)_vQ /tmp/tmp9LAnw8 [1] "/tmp/tmp9LAnw8" /tmp/tmpInteger(9)Anw8 /tmp/tmpvL13LQ [1] "/tmp/tmpvL13LQ" /tmp/tmpvLInteger(13)Q /tmp/tmpfB9L8g [1] "/tmp/tmpfB9L8g" /tmp/tmpfBInteger(9)8g /tmp/tmp0LtlQ6 [1] "/tmp/tmp0LtlQ6" /tmp/tmpInteger(0)tlQ6 /tmp/tmpQF4L5I [1] "/tmp/tmpQF4L5I" /tmp/tmpQFInteger(4)5I /tmp/tmp_0LOOr [1] "/tmp/tmp_0LOOr" /tmp/tmp_Integer(0)OOr /tmp/tmpdDDj7L [1] "/tmp/tmpdDDj7L" /tmp/tmpdDDjInteger(7) /tmp/tmpr0LiLu [1] "/tmp/tmpr0LiLu" /tmp/tmprInteger(0)iLu /tmp/tmpu5LnMJ [1] "/tmp/tmpu5LnMJ" /tmp/tmpuInteger(5)nMJ /tmp/tmp0Py04L [1] "/tmp/tmp0Py04L" /tmp/tmp0PyInteger(04) /tmp/tmp3LZjCk [1] "/tmp/tmp3LZjCk" /tmp/tmpInteger(3)ZjCk 17
on sage.math.
Attachments (1)
Change History (38)
comment:1 Changed 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
- Description modified (diff)
comment:3 Changed 9 years ago by
comment:4 Changed 9 years ago by
Using str
instead of sageobj
seems to work. Of course, that doesn't explain the behavior of sageobj
here.
Aside: Would we avoid the platform-dependent /private
prefix with sage.misc.misc.tmp_dir
? The sage-cleaner
would later delete the directory.
comment:5 follow-up: ↓ 6 Changed 9 years ago by
Ticket #10264 is an apparent duplicate. Jeroen Demeyer has attached a patch there.
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 9 years ago by
comment:7 in reply to: ↑ 6 Changed 9 years ago by
- Milestone set to sage-4.6.1
comment:8 follow-up: ↓ 9 Changed 9 years ago by
If you look carefully, you notice that these errors happen exactly when there is a digit followed by the letter "L".
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 9 years ago by
Replying to jdemeyer:
If you look carefully, you notice that these errors happen exactly when there is a digit followed by the letter "L".
Hah! You got it nailed -
/tmp/tmp3Vd1Lg [1] "/private/tmp/tmp3Vd1Lg" /private/tmp/tmp3VdInteger(1)g
the 1L
becomes Integer(1)
. And all of them do that.
So it thinks we have a 'long integer' when in fact it's just a random directory. I think maybe this is a problem in R itself, in setwd
?
comment:10 in reply to: ↑ 9 Changed 9 years ago by
- Keywords r added
Replying to kcrisman:
I think maybe this is a problem in R itself, in
setwd
?
I doubt it, since r.getwd()
returns the right thing, but sageobj(r.getwd())
not.
comment:11 follow-up: ↓ 12 Changed 9 years ago by
If I comment out line 1717 in interfaces/r.py
# Change 'dL' to 'Integer(d)' exp = rel_re_integer.sub(self._subs_integer, exp)
(in sage.interfaces.r.RElement._sage_
), I get no failures with the script in the description.
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 13 Changed 9 years ago by
Replying to mpatel:
If I comment out line 1717 in
interfaces/r.py
# Change 'dL' to 'Integer(d)' exp = rel_re_integer.sub(self._subs_integer, exp)(in
sage.interfaces.r.RElement._sage_
), I get no failures with the script in the description.
But I suppose you might get errors somewhere else?
comment:13 in reply to: ↑ 12 Changed 9 years ago by
Replying to jdemeyer:
Replying to mpatel:
If I comment out line 1717 in
interfaces/r.py
# Change 'dL' to 'Integer(d)' exp = rel_re_integer.sub(self._subs_integer, exp)(in
sage.interfaces.r.RElement._sage_
), I get no failures with the script in the description.But I suppose you might get errors somewhere else?
You're probably right. I was just following up on your observation about long integers with an experiment.
How about
sage: '"%s"' % os.path.realpath(tmpdir) == r.eval('dput(%s)' % r.getwd().name()) True
? Can we simplify this?
comment:14 Changed 9 years ago by
Or
sage: os.path.realpath(tmpdir) in str(r.getwd()) True
?
comment:15 follow-up: ↓ 16 Changed 9 years ago by
Personally, I am against changing doctests when the underlying functions are broken. In this case, it is clearly the conversion from R to Sage which is broken, not the doctest. By changing the doctest, we just hide the bug under the carpet instead of solving it.
comment:16 in reply to: ↑ 15 Changed 9 years ago by
Replying to jdemeyer:
Personally, I am against changing doctests when the underlying functions are broken. In this case, it is clearly the conversion from R to Sage which is broken, not the doctest. By changing the doctest, we just hide the bug under the carpet instead of solving it.
+1, though the doctest tries to test something else as I understand it.
comment:17 follow-up: ↓ 18 Changed 9 years ago by
- Status changed from new to needs_work
I have a patch that should fix this, but due to lack of knowledge of R I can't effectively test it. I think it needs some more doctests for the _sage_
function.
Changed 9 years ago by
comment:18 in reply to: ↑ 17 Changed 9 years ago by
Replying to wjp:
I have a patch that should fix this, but due to lack of knowledge of R I can't effectively test it. I think it needs some more doctests for the
_sage_
function.
I will eventually look at this, but because it's a rather harmless doctest failure (even though we really don't like doctest failures) I won't be able to check it out very quickly.
I'm also putting your request from sage-devel here for reference as to what you think is needed for review - thanks!
If somebody more familiar with the R language and interface could take a look if the string-related corner cases work, and if it didn't break anything non-string-related (and maybe add some more doctests to the _sage_ method), that would be great.
comment:19 Changed 7 years ago by
- Priority changed from major to critical
This is still a very annoying bug.
comment:20 Changed 6 years ago by
- Cc nthiery added
- Status changed from needs_work to needs_info
By #12415, a remark was added in r.py, pointing to this ticket:
sage: os.path.realpath(tmpdir) == sageobj(r.getwd()) # known bug (:trac:`9970`) True
Note that in #12876, Nicolas changes the test by inserting os.path.realpath on the right side as well, hence:
sage: os.path.realpath(tmpdir) == os.path.realpath(sageobj(r.getwd())) True
I don't know if this test is all what this ticket is about. So, please decide yourself if this ticket became a duplicate (or actually a sub-problem) of #12876
comment:21 follow-up: ↓ 22 Changed 6 years ago by
Unrelated as far as I can tell.
comment:22 in reply to: ↑ 21 ; follow-up: ↓ 24 Changed 6 years ago by
- Status changed from needs_info to needs_review
Replying to wjp:
Unrelated as far as I can tell.
OK, then the ticket status should probably reverted (it has been "needs work").
In any case, it seems that #12876 needs this change to make the doctest failure vanish. I suggest that a "todo" is added, referring to this ticket, stating that the doctest fix from #12876 is not a fix of the underlying problem, which is dealt with here.
comment:23 Changed 6 years ago by
- Status changed from needs_review to needs_work
comment:24 in reply to: ↑ 22 ; follow-up: ↓ 25 Changed 6 years ago by
comment:25 in reply to: ↑ 24 ; follow-up: ↓ 30 Changed 6 years ago by
Replying to jdemeyer:
Replying to SimonKing:
In any case, it seems that #12876 needs this change to make the doctest failure vanish.
Are you sure, because I don't see what #12876 has to do with either the conversion from R to Sage or with filesystem paths.
Well, at least it used to need it. If I recall correctly, at the time when I created the first version of this patch, we got a consistent error in the test, and with the patch the error consistently vanished.
However, as I've learned today, the tests also pass without this patch. So, we should drop it.
comment:26 Changed 6 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:27 Changed 6 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:28 Changed 6 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:29 Changed 5 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:30 in reply to: ↑ 25 ; follow-up: ↓ 31 Changed 5 years ago by
- Status changed from needs_work to needs_info
Replying to SimonKing:
However, as I've learned today, the tests also pass without this patch. So, we should drop it.
So should this ticket be closed as invalid?
comment:31 in reply to: ↑ 30 Changed 5 years ago by
comment:32 Changed 5 years ago by
- Description modified (diff)
comment:33 Changed 5 years ago by
- Status changed from needs_info to needs_work
comment:34 Changed 5 years ago by
- Milestone changed from sage-6.4 to sage-6.5
comment:35 Changed 11 months ago by
As of Sage 8.5 I no longer get errors from the script above, so presumably "known bug" can be removed.
However, r.chdir
and os.chdir
now do the same thing, i.e. the current directory for R and Sage is the same, which suggests to me that this function should be deprecated entirely. Unless it is desirable to be able to set different directories, in which case this has to be reimplemented.
comment:36 Changed 10 months ago by
- Cc gh-timokau added
Timo - am I right that with rpy2 interface the working directory of python and R are always the same and so r.chdir
does not really make sense anymore?
comment:37 Changed 10 months ago by
Yes that is right. I wasn't aware that there was a difference before. I think sharing a working directory is actually less confusing. You're right that r.chdir
should be removed if we don't want to restore the old behaviour.
That's very interesting, thanks for pointing it out. Since the
r.chdir()
is a pretty recent addition, it's not surprising there are unusual bugs with this. I get the same behavior on Mac OS X 10.4, so this is not platform dependent, unsurprisingly.in printing out the first ones until I get a failure (change the
if
loop to awhile
loop, basically), and a second timeso that can't be the issue, at least not by itself. The position of the integer seems irrelevant as well.