Add -l option to %R magic to allow passing in of local namespace #2130

Merged
merged 1 commit into from Aug 21, 2012

Projects

None yet

4 participants

@guyhf
Contributor
guyhf commented Jul 12, 2012

Feature request for rmagic: It would be great to be able to pass in local variables to the %R magic so that you can write functions that invoke R with values from the local scope (not just from user_ns). For example:

%load_ext rmagic
import numpy as np
def fun(values):
    for v in values:
        %R -i v -o prob prob <- pnorm(v)
        print prob
fun(np.arange(-1,1,.25))

Right now with %R -i magic you can only get variables from user_ns, so the above code won't work. I have added a %R -l argument that works just like %R -i except that variables are taken from the local scope. So the above code would be written:

%load_ext rmagic
import numpy as np
def fun(values):
    for v in values:
        %R -l v -o prob prob <- pnorm(v)
        print prob
fun(np.arange(-1,1,.25))

And the result would be:

[ 0.15865525]
[ 0.22662735]
[ 0.30853754]
[ 0.40129367]
[ 0.5]
[ 0.59870633]
[ 0.69146246]
[ 0.77337265]

The following changes to rmagic.py would make this work.

guyhf@5448c93

@takluyver
Member

Rather than adding a separate option, why not make -i input variables look in the local scope first? That's simpler for the user and fits in with how we already use variables.

P.S. It looks like your editor is deleting trailing spaces. If you want to make a pull request, can you disable that, so that all the extra changes don't clutter up the diff? Thanks.

@takluyver
Member

Ah, I see that you did those things. Github oddity: we get notified of comments, but not of new commits on a pull request, so remember to say something when you update it ;-)

I think this should also have a test - look in IPython.extensions.tests.test_rmagic for examples. It will be a little more complex than that, because it needs to define a function within IPython. This might be one time when it's easiest to use a doctest - see e.g. IPython.core.tests.test_magic.doctest_time.

@guyhf
Contributor
guyhf commented Jul 23, 2012

OK, I amended the commit -- and also added a parallel change to the %octave magic, which shares the same -i syntax.

On Jul 16, 2012, at 10:03 AM, Thomas Kluyver reply@reply.github.com wrote:

Ah, I see that you did those things. Github oddity: we get notified of comments, but not of new commits on a pull request, so remember to say something when you update it ;-)

I think this should also have a test - look in IPython.extensions.tests.test_rmagic for examples. It will be a little more complex than that, because it needs to define a function within IPython. This might be one time when it's easiest to use a doctest - see e.g. IPython.core.tests.test_magic.doctest_time.


Reply to this email directly or view it on GitHub:
#2130 (comment)

@takluyver
Member

Test results for commit 1dea0ba merged into master
Platform: linux2

  • python2.7: OK (libraries not available: oct2py rpy2)
  • python3.2: OK (libraries not available: cython oct2py pymongo rpy2 wx wx.aui)

Not available for testing: python2.6

@bfroehle
Contributor

Looks pretty good to me, but I don't use R regularly enough to test this.

Outside of the scope of this pull request, I must comment that the @needs_local_scope decorator is a little clumsy. It'd be nicer if it just passed in local_ns as a keyword argument. However at least it doesn't directly conflict with the cell magics (as I guess there isn't a sense of a local namespace in the cell magics).

@takluyver
Member

I think the decorator is a case of explicit is better than implicit
(where parsing the function signature for a particular parameter is
implicit).

@bfroehle
Contributor

Yes, I don't think we should parse the function signature, just that the namespace should be passed in the kwargs instead of the args, making usage a simpler:

@needs_local_scope 
...
def magic_function(line, cell=None, local_ns=None):
    ...

But this is just bike shedding and not related to this issue.

@takluyver
Member

Ah, that makes sense, then. Yes, that's probably a good idea.

@guyhf
Contributor
guyhf commented Aug 3, 2012

I modified the needs_local_scope decorator so that it passes in a local_ns kwarg instead. This makes for a cleaner interface. I tested it with R, but I don't use octave so I couldn't run those tests.

@takluyver takluyver commented on an outdated diff Aug 4, 2012
IPython/extensions/octavemagic.py
if args.input:
for input in ','.join(args.input).split(','):
input = unicode_to_str(input)
- self._oct.put(input, self.shell.user_ns[input])
+ self._oct.put(input, local_ns.get(input, self.shell.user_ns.get(input, None)))
@takluyver
takluyver Aug 4, 2012 Member

There's a change in behaviour here if the user specifies an input variable that doesn't exist - before it would fail with a KeyError, now it will use None (or the octave equivalent) instead. I think failing is right, because it probably means the user mistyped something. It might take a couple of extra lines to get it to fail properly, though.

@takluyver takluyver commented on an outdated diff Aug 4, 2012
IPython/extensions/rmagic.py
if args.input:
for input in ','.join(args.input).split(','):
- self.r.assign(input, self.pyconverter(self.shell.user_ns[input]))
+ self.r.assign(input, self.pyconverter(local_ns.get(input, self.shell.user_ns.get(input, None))))
@takluyver
takluyver Aug 4, 2012 Member

As above, I think it should fail if the input variable doesn't exist.

@guyhf
Contributor
guyhf commented Aug 4, 2012

That makes sense. I just switched it from user_ns.get() to back to user_ns[] will cause the same KeyError as before.

@takluyver
Member

Except I think that will throw the error if you pass a variable that only exists in the local namespace, because the second argument to .get() is evaluated unconditionally. I still think it will take a couple of extra lines to do it correctly. Something like:

try:
    val = local_ns[input]
except KeyError:
    val = self.shell.user_ns[input]
self.r.assign(input, self.pyconverter(val))

(Python 3.3 has a handy ChainMap class that will make this sort of thing easier, but for now we have to do it the long way.)

@guyhf
Contributor
guyhf commented Aug 5, 2012

Thanks. I fixed that and added some doctests to cover that case.

@takluyver takluyver commented on an outdated diff Aug 5, 2012
IPython/extensions/tests/test_rmagic.py
+ ...: %R -i u -o result result <- u+1
+ ...: return result[0]
+ ...:
+
+In [6]: test_rmagic_localscope(1) == 2
+Out[6]: True
+
+In [7]: e = None
+
+In [8]: try:
+ ...: %R -i var_not_defined -o result result = var_not_defined
+ ...: except Exception as e:
+ ...: pass
+ ...:
+
+In [9]: isinstance(e, KeyError)
@takluyver
takluyver Aug 5, 2012 Member

I think this will fail in Python 3 - e only exists within the except clause, so it's a NameError to refer to it here. This is the sort of thing I don't like about doctests - they're quite brittle.

Ideally, we could convert this into a standard unit test - use ip.run_cell to define the function, ip.run_line_magic to call %R and nt.assert_raises to check that this bit throws a KeyError.

@bfroehle bfroehle commented on an outdated diff Aug 21, 2012
IPython/extensions/tests/test_octavemagic.py
+In [5]: def test_octavemagic_localscope(u):
+ ...: %octave -i u -o result result = u + 1
+ ...: return result
+ ...:
+
+In [6]: result_ls = test_octavemagic_localscope(1)
+result = 2
+
+In [7]: result_ls == 2
+Out[7]: True
+
+In [8]: e = None
+
+In [9]: try:
+ ...: %octave -i var_not_defined -o result result = var_not_defined
+ ...: except Exception as e:
@bfroehle
bfroehle Aug 21, 2012 Contributor

Alternatively I think you can do this in a Python 3 portable way as:

except:
    e = sys.exc_info()[1]

(And add import sys to the top of the file if necessary.)

@bfroehle
Contributor

Pinging @guyhf. Just a few more quick changes here and I think this is ready to be committed.

@guyhf
Contributor
guyhf commented Aug 21, 2012

Just updating it now. I'll convert the KeyError exception test to a standard unit test.

@travisbot

This pull request passes (merged 80f3b364 into 3c8d448).

@guyhf
Contributor
guyhf commented Aug 21, 2012

I changed the KeyError test to a unit test and tested both IPython.extensions.tests.test_rmagic and IPython.extensions.tests.test_octavemagic. I'm fine changing the other doctests to unit tests if that is what is preferred.

@takluyver
Member

That looks fine.

I prefer unit tests because I've often had to fight with both doctests and the machinery that runs them (if you ever want a free headache, look at IPython/testing/plugin/ipdoctest.py). But I know they're easier to write, so as long as they work, there's no requirement to change them.

@bfroehle: I'll let you have another look at this, but if you're happy, you can merge it.

@guyhf
Contributor
guyhf commented Aug 21, 2012

I went ahead and changed the rest of the doctests to unit tests and re-checked IPython.extensions.tests.test_rmagic and IPython.extensions.tests.test_octavemagic.

@travisbot

This pull request passes (merged 5684165a into 3c7c235).

@takluyver takluyver commented on an outdated diff Aug 21, 2012
IPython/extensions/tests/test_octavemagic.py
@@ -62,3 +62,24 @@ def test_octave_plot():
'plot([1, 2, 3]); figure; plot([4, 5, 6]);')
nt.assert_equal(test_octave_plot.svgs_generated, 2)
+
+def test_octavemagic_localscope():
+ ip = get_ipython()
+ ip.magic('load_ext octavemagic')
+ ip.push({'x':0})
+ ip.run_line_magic('octave', '-i x -o result result = x+1')
+ result = ip.user_ns['result']
+ np.testing.assert_equal(result, 1)
+
+ ip.run_cell('''def test_octavemagic_localscope(u):
@takluyver
takluyver Aug 21, 2012 Member

Let's call this function something different from the test, to avoid confusion. Also, while we're at it, let's not have 'test' in the name, just in case nose somehow finds it (it attempts to run any function it finds named 'test').

@takluyver takluyver commented on an outdated diff Aug 21, 2012
IPython/extensions/tests/test_rmagic.py
@@ -60,3 +60,23 @@ def test_cell_magic():
ip.run_cell_magic('R', '-i x,y -o r,xc a=lm(y~x)', snippet)
np.testing.assert_almost_equal(ip.user_ns['xc'], [3.2, 0.9])
np.testing.assert_almost_equal(ip.user_ns['r'], np.array([-0.2, 0.9, -1. , 0.1, 0.2]))
+
+
+def test_rmagic_localscope():
+ ip.push({'x':0})
+ ip.run_line_magic('R', '-i x -o result result <-x+1')
+ result = ip.user_ns['result']
+ np.testing.assert_equal(result[0], 1)
+
+ ip.run_cell('''def test_rmagic_localscope(u):
@takluyver
takluyver Aug 21, 2012 Member

Same with this one, please.

@bfroehle
Contributor

Test results for commit 5684165 merged into master (3c7c235)
Platform: linux2

  • python2.6: OK (libraries not available: azure matplotlib oct2py pymongo qt rpy2 wx wx.aui)
  • python2.7: OK (libraries not available: azure)
  • python3.2: OK (libraries not available: azure oct2py pymongo rpy2 wx wx.aui)

Not available for testing:

@bfroehle
Contributor

With those small changes that @takluyver suggests in the tests, I think it's ready to go.

@travisbot

This pull request passes (merged 7d7a7118 into 3c7c235).

@guyhf
Contributor
guyhf commented Aug 21, 2012

I renamed the internal test functions.

@bfroehle bfroehle commented on the diff Aug 21, 2012
IPython/extensions/octavemagic.py
if args.input:
for input in ','.join(args.input).split(','):
input = unicode_to_str(input)
- self._oct.put(input, self.shell.user_ns[input])
+ try:
+ val = local_ns[input]
+ except KeyError:
+ val = self.shell.user_ns[input]
@bfroehle
bfroehle Aug 21, 2012 Contributor

Just occurred to me that this is more-or-less equivalent to eval(input, self.shell.user_ns, local_ns) (with the exception that it'll raise a NameError instead of a KeyError if the key cannot be found).

But I think I like the extended try/except block better. Certainly it's faster to execute.

@bfroehle bfroehle commented on the diff Aug 21, 2012
IPython/extensions/tests/test_octavemagic.py
@@ -62,3 +62,24 @@ def test_octave_plot():
'plot([1, 2, 3]); figure; plot([4, 5, 6]);')
nt.assert_equal(test_octave_plot.svgs_generated, 2)
+
+def test_octavemagic_localscope():
+ ip = get_ipython()
+ ip.magic('load_ext octavemagic')
@bfroehle
bfroehle Aug 21, 2012 Contributor

If we make any other changes this should probably be changed to ip.run_line_magic('load_ext', 'octavemagic'), but there are enough other instances of ip.magic in the test routines where it's clearly not actually an issue.

@bfroehle bfroehle and 1 other commented on an outdated diff Aug 21, 2012
IPython/extensions/tests/test_octavemagic.py
@@ -62,3 +62,24 @@ def test_octave_plot():
'plot([1, 2, 3]); figure; plot([4, 5, 6]);')
nt.assert_equal(test_octave_plot.svgs_generated, 2)
+
+def test_octavemagic_localscope():
+ ip = get_ipython()
+ ip.magic('load_ext octavemagic')
+ ip.push({'x':0})
+ ip.run_line_magic('octave', '-i x -o result result = x+1')
+ result = ip.user_ns['result']
+ np.testing.assert_equal(result, 1)
@bfroehle
bfroehle Aug 21, 2012 Contributor

Is there a reason why we are using numpy to do the equality testing?

@guyhf
guyhf Aug 21, 2012 Contributor

I was following the lead of the earlier tests. This one doesn't need it though.

@bfroehle
Contributor

Great. I think this could be merged now. :)

@guyhf guyhf Added functionality to %R and %octave magics so that -i first looks i…
…n local

scope before looking in user_ns.  This allows the magics to be called from
within functions.

Also Modified @needs_local_scope decorator for magic extensions to use a
kwarg with the local namespace, so functions now get a separate kward
local_ns, e.g.

def magic_function(line, cell=None, local_ns=None):
    ...
99483a2
@guyhf
Contributor
guyhf commented Aug 21, 2012

Switched np.testing.assert_equals to nt.assert_equals.

@travisbot

This pull request passes (merged 99483a2 into 3c7c235).

@takluyver
Member

Thanks, @guyhf .

@bfroehle : I'm happy with this. You can push the green button if you don't see any other issues.

@bfroehle bfroehle merged commit 5308c36 into ipython:master Aug 21, 2012

1 check passed

default The Travis build passed
Details
@bfroehle
Contributor

@guyhf Merged! Thanks for all your work and all your patience. I think we ended up with an excellent result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment