Opened 9 years ago

Last modified 11 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 jdemeyer)

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)

9970_r_strings.patch (2.5 KB) - added by wjp 9 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 9 years ago by mpatel

  • Description modified (diff)

comment:2 Changed 9 years ago by mpatel

  • Description modified (diff)

comment:3 Changed 9 years ago by kcrisman

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.

I am wondering whether it is simply that the directory names are preparsed or something with all those Integer things - maybe these 13-20 out of 1000 are the ones that have numeric characters in the alphanumeric string?. Though /tmp/tmp0Py04L only has the 04 in Integer, not the first 0. Also, I get

/tmp/tmpooH_dP [1] "/private/tmp/tmpooH_dP" /private/tmp/tmpooH_dP
/tmp/tmpLPmXNF [1] "/private/tmp/tmpLPmXNF" /private/tmp/tmpLPmXNF
/tmp/tmpH7BqRp [1] "/private/tmp/tmpH7BqRp" /private/tmp/tmpH7BqRp
/tmp/tmpmV9yGJ [1] "/private/tmp/tmpmV9yGJ" /private/tmp/tmpmV9yGJ
/tmp/tmpZig4LH [1] "/private/tmp/tmpZig4LH" /private/tmp/tmpZigInteger(4)H

in printing out the first ones until I get a failure (change the if loop to a while loop, basically), and a second time

/tmp/tmp2Ny1fm [1] "/private/tmp/tmp2Ny1fm" /private/tmp/tmp2Ny1fm
/tmp/tmpY6qCbW [1] "/private/tmp/tmpY6qCbW" /private/tmp/tmpY6qCbW
/tmp/tmpQWhSyG [1] "/private/tmp/tmpQWhSyG" /private/tmp/tmpQWhSyG
/tmp/tmpJnST6Z [1] "/private/tmp/tmpJnST6Z" /private/tmp/tmpJnST6Z
/tmp/tmpiP3g5g [1] "/private/tmp/tmpiP3g5g" /private/tmp/tmpiP3g5g
/tmp/tmpBo_DwU [1] "/private/tmp/tmpBo_DwU" /private/tmp/tmpBo_DwU
/tmp/tmp0O2kjX [1] "/private/tmp/tmp0O2kjX" /private/tmp/tmp0O2kjX
/tmp/tmpJzFFFs [1] "/private/tmp/tmpJzFFFs" /private/tmp/tmpJzFFFs
/tmp/tmpF3eRCC [1] "/private/tmp/tmpF3eRCC" /private/tmp/tmpF3eRCC
/tmp/tmp3Vd1Lg [1] "/private/tmp/tmp3Vd1Lg" /private/tmp/tmp3VdInteger(1)g

so that can't be the issue, at least not by itself. The position of the integer seems irrelevant as well.

comment:4 Changed 9 years ago by mpatel

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: Changed 9 years ago by mpatel

Ticket #10264 is an apparent duplicate. Jeroen Demeyer has attached a patch there.

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

Replying to mpatel:

Ticket #10264 is an apparent duplicate. Jeroen Demeyer has attached a patch there.

No, it seems to be a different problem with the same doctest.

comment:7 in reply to: ↑ 6 Changed 9 years ago by jdemeyer

  • Milestone set to sage-4.6.1

Replying to mpatel:

No, it seems to be a different problem with the same doctest.

Exactly. However, in order not to get into merge conflicts, I propose that any patch for this ticket actually goes to #10264.

comment:8 follow-up: Changed 9 years ago by jdemeyer

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: Changed 9 years ago by kcrisman

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 jdemeyer

  • 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: Changed 9 years ago by 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.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 9 years ago by 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?

comment:13 in reply to: ↑ 12 Changed 9 years ago by mpatel

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 mpatel

Or

sage: os.path.realpath(tmpdir) in str(r.getwd())
True

?

comment:15 follow-up: Changed 9 years ago by 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.

comment:16 in reply to: ↑ 15 Changed 9 years ago by leif

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: Changed 9 years ago by wjp

  • Authors set to Willem Jan Palenstijn
  • 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 wjp

comment:18 in reply to: ↑ 17 Changed 9 years ago by kcrisman

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 jdemeyer

  • Priority changed from major to critical

This is still a very annoying bug.

comment:20 Changed 7 years ago by SimonKing

  • 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: Changed 7 years ago by wjp

Unrelated as far as I can tell.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 7 years ago by SimonKing

  • 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 7 years ago by SimonKing

  • Status changed from needs_review to needs_work

comment:24 in reply to: ↑ 22 ; follow-up: Changed 7 years ago by 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.

comment:25 in reply to: ↑ 24 ; follow-up: Changed 7 years ago by SimonKing

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 jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:27 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:28 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:29 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:30 in reply to: ↑ 25 ; follow-up: Changed 5 years ago by mmezzarobba

  • 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 jdemeyer

Replying to mmezzarobba:

So should this ticket be closed as invalid?

NO

comment:32 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:33 Changed 5 years ago by mmezzarobba

  • Status changed from needs_info to needs_work

comment:34 Changed 5 years ago by leif

  • Milestone changed from sage-6.4 to sage-6.5

comment:35 Changed 11 months ago by novoselt

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 11 months ago by novoselt

  • 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 11 months ago by gh-timokau

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.

Note: See TracTickets for help on using tickets.