Autocompletion: Fix issue #3723 -- ordering of completions for magic commands and variables with same name #3801

dpsanders · 2013-07-26T23:25:03Z

[ not for 1.0 ]

This PR fixes issue #3723.

The introduction of the %matplotlib magic broke the order of autocompletions in the sequence of commands described in #3723.

This PR fixes this by changing the order in which the completions are returned by the complete() function, by
introducing a custom key, called penalize_magics_key, which is passed to sorted to sort the various lists of completions assembled from different places.

penalize_magics_key moves the initial % or %% to the end of the key, so that the lexicographic string ordering gives "matplotlib" < "matplotlib%" < "matplotlib%%", but all of these are together in the completion list.

Added a test function test_magic_completion_order which tests that these orderings are correct.

Carreau · 2013-07-26T23:50:23Z

IPython/core/completer.py

+    pattern = re.compile("%+")  # find %
+    m = pattern.match(word) # at the start of the string
+
+    print m


forgotten print.

dpsanders · 2013-07-27T00:43:45Z

Requested changes have now been implemented.

Ready to merge.

Carreau · 2013-07-27T01:12:58Z

IPython/core/completer.py

@@ -177,6 +181,52 @@ def compress_user(path, tilde_expand, tilde_val):
        return path


+def uniquify(seq):
+    """uniquify a list
+    i.e. remove duplicate elements, but preserving original order


One blank line.

This uniquify function is not used in the current version, but it is an
alternative ordering method.
Should I just remove it?

On Fri, Jul 26, 2013 at 6:13 PM, Matthias Bussonnier <
notifications@github.com> wrote:

In IPython/core/completer.py:

@@ -177,6 +181,52 @@ def compress_user(path, tilde_expand, tilde_val):
return path

+def uniquify(seq):

"""uniquify a list

i.e. remove duplicate elements, but preserving original order

One blank line.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3801/files#r5436317
.

Dr. David P. Sanders

Profesor Titular "A" / Associate Professor
Departamento de Física, Facultad de Ciencias
Universidad Nacional Autónoma de México (UNAM)

dpsanders@gmail.com
http://sistemas.fciencias.unam.mx/~dsanders

Cubículo / office: #414, 4o. piso del Depto. de Física

Tel.: +52 55 5622 4965

Yes you can remove if it is not used.

Carreau · 2013-07-29T18:00:50Z

I would be in favor of a more general way of "ordering" completions. And I am uncomfortable to have it for 1.0.
Also, this is also probably something that could be done totally in the frontend.

So even if it make sens, I'll stay neutral on this one for the time beeing.

dpsanders · 2013-08-05T05:59:14Z

I believe this is ready to merge.

In my opinion, it should definitely go into 1.0 (despite @ivanov 's label) as a stop-gap measure until IPEP 11
is addressed. Otherwise people will be having problems completing on matplotlib for the next few months, which does not seem like a good idea!

ivanov · 2013-08-05T06:07:06Z

it's not that big of a problem - most of the time we deal with plt when calling plots anyway.

The reason this is marked for 2.0 is because we already cut an alpha, and then a release candidate for 1.0, and are getting feedback from people who are testing this heavily so that we can release 1.0 that we're fairly confident doesn't have anything majorly broken, and whatever limitations there are should be documented. Completion is a pretty fundamental part of IPython, and delaying this until after 1.0 will actually give us more room to play with it, try new things and really get this right.

dpsanders · 2013-08-05T06:08:22Z

OK, fair enough!

Carreau · 2013-08-05T08:16:54Z

might be of interest: http://docs.python.org/2/library/difflib.html#sequencematcher-examples

fperez · 2013-08-06T22:03:40Z

Yup, to reiterate @ivanov's comment above, there's absolutely no way this is 1.0 material. We're strictly in tiny-fixes and doc touch-up territory at this point.

takluyver · 2013-08-27T20:12:22Z

@rmcgibbo, you were working on refactoring the tab completion stuff (IPEP 11). Have you already looked at anything like this? is there a clean way to work it into what you're doing?

rmcgibbo · 2013-08-27T20:26:47Z

I really don't have the bandwidth to work on that proposal now. Sorry to leave you guys hanging.

takluyver · 2013-08-27T21:23:53Z

Not to worry. Feel free to come back and pick it up whenever you're ready.

ellisonbg · 2013-09-20T18:36:31Z

@takluyver and @dpsanders what do we want to do with this PR?

takluyver · 2013-09-20T18:52:53Z

I'm reluctant to stick yet more complexity into the completer system when we know it's in need of a refactor, but on the other hand we don't know when IPEP 11 might get completed. I guess it depends on how much we want this feature.

ellisonbg · 2013-09-20T20:01:15Z

I too am reluctant to add more complexity to the completer code. @fperez do you want to make the call on this one?

ellisonbg · 2013-09-23T21:24:09Z

We talked to @fperez last week and we want to move forward with this PR, even without the larger completer refactor. But, @fperez thinks this can be done without using regular expressions (maybe just startswith?). @dpsanders do you want to see if you can simplify it a bit?

dpsanders · 2013-09-23T21:28:46Z

@ellisonbg Apologies for the delay in replying. I'll give it a go when I have a free moment!

Basically all that it does is shift the initial % or %% of a magic to the end of the string, so this should indeed be pretty easy to do in a simple way.

dpsanders · 2013-09-23T22:13:45Z

Have a look at the code in the very first commit of this PR. Is that the kind of thing you mean?:

 if word[0] == "%":  # if it's a magic command     
    return word[1:] + "%"

ellisonbg · 2013-09-23T22:15:25Z

Or:

if word.startswith('%'):
    return word.lstrip('%')

dpsanders · 2013-09-23T22:17:23Z

Nice, thanks!

fperez · 2013-09-25T01:54:09Z

Quick note, the code suggested above by @ellisonbg won't quite do the correct penalization, as it will make %foo sort identical to foo, but we want it to sort after foo. We do need to put the % characters up at the end, as @dpsanders' original code did.

This alternative does the right thing:

def penalize_magics_key2(word):
    i = word.rfind('%')
    if i in (0,1):
        s = i+1
        return word[s:]+word[:s]
    else:
        return word

and in fact has better behavior than the one proposed here in some edge cases. I wrote up a quick analysis with checks and performance in a notebook so @dpsanders can check.

I would suggest using that code (appropriately commented explaining what's going on) instead. It has better behavior and is ~ 2x faster, in addition to being easier to understand for someone who's not a regular expression expert, since it only involves indexing into strings.

fperez · 2013-09-25T01:54:18Z

Ah, and one more thing: this PR needs a rebase.

dpsanders · 2013-09-26T05:45:25Z

@fperez Great, thanks. I hope to get to it in the next couple of days, or otherwise at the weekend.

dpsanders · 2013-09-27T04:10:36Z

@fperez Very nice notebook, thanks.
For some reason, your new version is not as fast on my machine.

I tried my original idea again -- just testing the first couple of characters and moving them to the end if necessary.
It's actually even faster, and arguably the code is clearer (though certainly less elegant!).
There are two versions, with and without testing for other % signs later in the string:

https://gist.github.com/6723952

(The final version of the code had a %timeit of 325 ns, which is in the notebook but for some reason does not seem to appear on nbviewer.)

Which version do you prefer?

fperez · 2013-09-27T22:36:19Z

Sure, we can go with that last one, it's actually easier to understand (clarity matters much more than performance here).

Note however that you must change w[0] to w[:1], otherwise it will throw an IndexError if the empty string is ever accessed for some reason (it shouldn't, but no point in leaving a crash path there waiting to be triggered).

Also keep in mind the need for a rebase.

…ordering.

…p to the start of the module so that it is not compiled each time

… namespace as a standard variable, to avoid problems if matplotlib is not available to import

…al. Corrected test

…laced the code with the simplest string-based version, removing the regex.

dpsanders · 2013-09-28T05:48:23Z

I have put the new code up but I am not understanding how/what to do for the rebase.
I am trying git rebase master,
but every commit from the branch seems to cause a conflict, and I don't understand how I should resolve them.
(I have looked at several tutorials, but without much success, sorry!)

…laced the code with the simplest string-based version, removing the regex.

dpsanders · 2013-09-28T06:15:11Z

I skipped all the commits except the last one for the rebase. (Is this legitimate...?)
This seems to have worked.
I have no idea why my username suddenly changed for the last commit...

Carreau · 2013-09-28T07:57:42Z

IPython/core/completer.py

@@ -499,6 +536,7 @@ def __init__(self, shell=None, namespace=None, global_namespace=None,
                         self.file_matches,
                         self.magic_matches,
                         self.python_func_kw_matches,
+                         self.alias_matches,


This dies not exist apparently. Due to @takluyver alias refactoring not so long ago ?

Carreau · 2013-09-28T08:00:35Z

Your user name probably change because you did some commits on another machine where your git user name is not correctly set up. The user will match the email that is registered with $ git config user.email.

dpsanders · 2013-09-28T18:32:50Z

I finally just reverted to the current master and reimplemented on top.

I seem to have made a mess with my github accounts though...

Carreau · 2013-09-28T18:48:35Z

computo-fc seem to be dpsanders(at)ciencias.unam.mx
dpsanders is dpsanders(at)gmail.com

If it can helps you.

dpsanders · 2013-09-28T19:00:40Z

Yes, thanks, I think that was the problem. And at some point I set the global user.name instead of the local one on the relevant repository.

ellisonbg · 2013-10-09T18:38:22Z

@dpsanders is this PR ready for more review?

dpsanders · 2013-10-25T03:18:00Z

Sorry for the delay in replying. From my point of view it's finished, so yes please go ahead and review.

dpsanders · 2013-10-25T03:18:26Z

Of course, by now it probably needs rebasing again!

Carreau · 2013-10-25T05:24:06Z

No need to rebase

Carreau · 2013-10-26T08:39:24Z

Looks good to me.
From what I re-read all comment have been adressed.

minrk · 2013-10-28T22:30:06Z

Merged, thanks!

dpsanders · 2013-10-28T22:33:39Z

:) Thanks, Min!

dpsanders · 2013-10-28T23:04:44Z

And thanks to everybody for comments and reviewing.

Autocompletion: ordering of completions for magic commands and variables with same name closes ipython#3723 closes ipython#3801

Carreau reviewed Jul 26, 2013
View reviewed changes

Carreau reviewed Jul 27, 2013
View reviewed changes

Modified code completion so that magic commands are penalized in the …

f537493

…ordering.

dpsanders added 6 commits September 27, 2013 23:48

Added test_complete function to test the ordering of magics

419144a

Reverted javascript that is not relevant to this PR

b03b68a

Removed an extra print and commented-out code. Moved the magics regex…

206cc8a

…p to the start of the module so that it is not compiled each time

Replaced matplotlib import with simply injecting variable called into…

9ae7ceb

… namespace as a standard variable, to avoid problems if matplotlib is not available to import

Removed uniquify (no longer used). Returned completion list to origin…

8bfb326

…al. Corrected test

Modified completer.py according to discussion on the original PR. Rep…

ee9f10f

…laced the code with the simplest string-based version, removing the regex.

dpsanders and others added 2 commits September 28, 2013 01:11

Modified completer.py according to discussion on the original PR. Rep…

67f6160

…laced the code with the simplest string-based version, removing the regex.

Fixed conflict

15ec864

Carreau reviewed Sep 28, 2013
View reviewed changes

computo-fc added 4 commits September 28, 2013 13:22

Reverted to current master

239a468

Reimplemented penalize_magics_key

5e2c471

Reverted test_completer to current master

ec6f0e1

Reimplemented test_magic_completion_order in test_completer.py

37fcad7

minrk closed this in 689935c Oct 28, 2013

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge Pull Request ipython#3801

2de1e66

Autocompletion: ordering of completions for magic commands and variables with same name closes ipython#3723 closes ipython#3801

Autocompletion: Fix issue #3723 -- ordering of completions for magic commands and variables with same name #3801

Autocompletion: Fix issue #3723 -- ordering of completions for magic commands and variables with same name #3801

Conversation

dpsanders commented Jul 26, 2013

Carreau Jul 26, 2013

Choose a reason for hiding this comment

dpsanders commented Jul 27, 2013

Carreau Jul 27, 2013

Choose a reason for hiding this comment

dpsanders Jul 27, 2013

Choose a reason for hiding this comment

Carreau Jul 29, 2013

Choose a reason for hiding this comment

dpsanders Aug 5, 2013

Choose a reason for hiding this comment

Carreau commented Jul 29, 2013

dpsanders commented Aug 5, 2013

ivanov commented Aug 5, 2013

dpsanders commented Aug 5, 2013

Carreau commented Aug 5, 2013

fperez commented Aug 6, 2013

takluyver commented Aug 27, 2013

rmcgibbo commented Aug 27, 2013

takluyver commented Aug 27, 2013

ellisonbg commented Sep 20, 2013

takluyver commented Sep 20, 2013

ellisonbg commented Sep 20, 2013

ellisonbg commented Sep 23, 2013

dpsanders commented Sep 23, 2013

dpsanders commented Sep 23, 2013

ellisonbg commented Sep 23, 2013

dpsanders commented Sep 23, 2013

fperez commented Sep 25, 2013

fperez commented Sep 25, 2013

dpsanders commented Sep 26, 2013

dpsanders commented Sep 27, 2013

fperez commented Sep 27, 2013

dpsanders commented Sep 28, 2013

dpsanders commented Sep 28, 2013

Carreau Sep 28, 2013

Choose a reason for hiding this comment

Carreau commented Sep 28, 2013

dpsanders commented Sep 28, 2013

Carreau commented Sep 28, 2013

dpsanders commented Sep 28, 2013

ellisonbg commented Oct 9, 2013

dpsanders commented Oct 25, 2013

dpsanders commented Oct 25, 2013

Carreau commented Oct 25, 2013

Carreau commented Oct 26, 2013

minrk commented Oct 28, 2013

dpsanders commented Oct 28, 2013

dpsanders commented Oct 28, 2013