Better slicing with ellipsis support by vodik · Pull Request #1493 · hylang/hy

vodik · 2018-01-30T19:51:22Z

Proof that its possible to optimize slicing. This code:

(setv foo [1 2 3])
(get foo (slice 1 2))
(slice 1 2)

Is now equivalent to:

foo = [1, 2, 3]
foo[1:2]
slice(1, 2)

I've also demoted cut to a macro as its now redundant, but imho still a useful form (still nicer to use when updating a list in place: (setv (cut foo) [1 2 3])

vodik · 2018-01-30T19:53:15Z

See #1492, but still not there yet as this is only concerned with a single dimension. Maybe the nicest would be to let get take a variable number of arguments...

gilch · 2018-01-30T19:58:13Z

get does take a variable number of arguments. But that means a chain of lookups, not one slice over multiple dimensions.

vodik · 2018-01-30T20:51:20Z

~~Oh, so like itemgetter?~~ Interesting, didn't know that.

Beyond that, though, any complains with this approach?

Kodiologist · 2018-01-31T00:12:15Z


-        indices, ret, _ = self._compile_collect(expr[1:])
-        ret += self.compile(expr[0])
+        if isinstance(expr[1], HyExpression) and expr[1][0] == "slice":


This will trigger on the string "slice" instead of just the symbol. You may also need to check that expr[1] is nonempty.

Kodiologist · 2018-01-31T00:15:31Z

Don't forget about those test failures.

Kodiologist · 2018-01-31T00:21:56Z

One issue with this approach is that slice isn't a reserved word, so there's no way to tell at compile-time that the symbol slice corresponds to __builtins__.slice.

vodik · 2018-01-31T04:44:17Z

One issue with this approach is that slice isn't a reserved word, so there's no way to tell at compile-time that the symbol slice corresponds to __builtins__.slice.

Oh right, is there any suggestions on how to deal with this? Should I promote slice to a builtin? Or is there another trick that I might not be aware of?

Kodiologist · 2018-01-31T05:13:04Z

slice is already a builtin, because Hy inherits Python's builtins. Making slice a special form just so we have the right of always defining what (slice ...) means, when in fact we intend it to expand to a regular function call whenever the compiler actually gets to a @builds("slice") method, is probably overkill. Perhaps the best approach is to check for __builtins__.slice instead. It's probably best to make cut emit this instead of a bare slice, anyway. While it's technically possible for the user to set __builtins__ to something weird, the double underscores in the name mean that this is officially a bad idea.

vodik · 2018-01-31T06:01:37Z

Perhaps the best approach is to check for __builtins__.slice instead.

So if I understand this correctly, I should compile the expression and then peek inside the results of that?

Kodiologist · 2018-01-31T06:14:24Z

No, I don't think you need to; I think it will suffice to check for the symbol __builtins__.slice. This won't work for something like (get foo ((. __builtins__ slice) ...)), but that's probably okay. It's just an optimization, after all.

Kodiologist · 2018-01-31T06:16:08Z

It should be possible to compile it first and check the AST instead of checking the symbol, but the code will probably be a bit wordier that way.

vodik · 2018-02-04T19:06:38Z

Right, so I'm not seeing an easy way to do this. Either I check the HyExpression/HySymbol for "slice", or I compile it first and look into the resulting ast.Name. But my understanding, and maybe this is where I'm missing something, but ast.Name is still going to result in runtime lookup, so there is no sensible to know for sure if builtins has been messed with.

Maybe I should add slice* to force a slice object to be emitted?

Or maybe this idea isn't workable then and we'll just have to live with the slightly less optimal code generation.

... to check for __builtins__.slice instead. It's probably best to make cut emit this instead of a bare slice, anyway.

cut still compiles to ast.Slice as it used to, so it doesn't matter what anyone has done to builtins. That's the only reason I moved it to a macro.

Kodiologist · 2018-02-04T19:14:46Z

It's true that both the name __builtins__ and the attribute slice of the referent have to be looked up at runtime. But what I'm saying is, the double underscores in the name imply that the user isn't supposed to reassign the name. So it's fair of us to assume it hasn't been reassigned. Does that make sense?

vodik · 2018-02-04T20:56:24Z

Ah, sorry, I've misunderstood you entirely.

Okay, so let me see if I'm following: you're suggesting that the cut macro emits __builtins__.slice instead of slice. Then I'll look for __builtins__.slice and emit ast.Slice in that case? Is this on top of rewriting slice too, but just to make it more bullet proof?

vodik · 2018-02-04T21:11:32Z

Okay, so if this is acceptable, I'll continue working on it. Currently get will either accept its current list of keys, or a single slice. I need to do some deeper refactoring to make the slice optimization work as expected, and then additionally dealing with #1492 and #1491 should be easy.

vodik · 2018-02-04T23:09:16Z

Okay, merged #1491 into this PR, need to just rework tests, but I think this should do the trick.

The following code compiled as expected on both 2 and 3:

(get foo (slice 1 2 3) ... bar)

will compile to

foo[1:2:3][...][bar]

The optimization currently falls apart when we use a tuple to have a multidimensional slice, but I think we can merge this and leave that as a separate PR.

Just need to fix the tests and add new ones...

Kodiologist · 2018-02-05T03:13:15Z

Okay, so let me see if I'm following: you're suggesting that the cut macro emits __builtins__.slice instead of slice. Then I'll look for __builtins__.slice and emit ast.Slice in that case?

Bingo.

Is this on top of rewriting slice too, but just to make it more bullet proof?

No, leave slice as a Python builtin.

Just need to fix the tests and add new ones...

Sounds good. Let us know when you want us to take a look again.

vodik · 2018-02-05T04:12:55Z

I think it can stand for review now. The code now properly emits the exact same AST that python does, multi or single dimension.

I took another step further and I deconstruct tuples when used as arguments to get to turn them into multi dimentional slices. This, as I understand it, is safe as Python represents them internally as tuples so there's no room for ambiguity. That means we now have the following code working:

(get foo 1 2 (slice 3 4) ... (slice 5 6))
(get foo (, 1 2 (slice 3 4) ... (slice 5 6)))

generating the following equivalent Python:

foo[1][2][3:4][...][5:6]
foo[(1), (2), 3:4, (...), 5:6]

The second example generates the following AST:

Module(
    body=[
        Expr(
            value=Subscript(value=Name(id='foo'),
                slice=ExtSlice(
                    dims=[Index(value=Num(n=1)),
                        Index(value=Num(n=2)),
                        Slice(lower=Num(n=3), upper=Num(n=4), step=None),
                        Index(value=Ellipsis),  # and just `Ellipsis` on PY2
                        Slice(lower=Num(n=5), upper=Num(n=6), step=None)])))])

This should put in place all the groundwork for the proposed #1481 change.

Kodiologist · 2018-02-05T21:31:07Z

+    return (isinstance(x, HyExpression)
+            and len(x) > 0
+            and isinstance(x[0], HySymbol)
+            and x[0] in (kind, "__builtin__" + kind))


Shouldn't this be x[0] == "__builtins__." + kind? You don't want to allow kind alone because then you'll trigger on slice alone, which might be rebound. Also, you need the period, and note the "s" in __builtins__.

Well __builtins__.slice doesn't work because __builtins__ is a dictionary, not an object. So we'd have to either change that too or write (get __builtins__ "slice"). And I'm not particularly eager to use that large expression down every time I want a slice to be slightly faster (and to be honest, same for __builtins__.slice).

If we don't feel comfortable with rewriting slice, I can just drop that entire part of the PR.

Well __builtins__.slice doesn't work because __builtins__ is a dictionary, not an object.

Are you sure?

$ python2 Python 2.7.14 (default, Sep 23 2017, 22:06:14) [GCC 7.2.0] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> __builtins__.slice <type 'slice'> $ python3 Python 3.6.3 (default, Oct 3 2017, 21:45:48) [GCC 7.2.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> __builtins__.slice <class 'slice'>

hy complains though:

=> (cut [1 2 3] 1 2 3) Traceback (most recent call last): File "/home/simon/src/hy/hy/importer.py", line 201, in hy_eval return eval(ast_compile(expr, "<eval>", "eval"), namespace) File "<eval>", line 1, in <module> AttributeError: 'dict' object has no attribute 'slice' => (type __builtins__) <class 'dict'>

Huh, that's a bizarre bug. We don't set __builtins__ anywhere in our code and yet Hy still sees a different object than Python does.

Oh shoot, that's a feature. Since we hope to be setting __main__ appropriately someday, we can't safely use get nor attribute lookup. I'm afraid I'm not sure what design would make sense, then.

I'm honestly quite comfortable letting slice mean slice here and just documenting the behaviour. We already prevent slice from being assigned to. Overriding __builtins__.slice in Python also won't change the behaviour of the : operator.

I could also add a check to make sure no slice macro has been defined and/or add slice* to force the construction of a slice object and that probable covers 99% of weird use cases.

We already prevent slice from being assigned to.

Are you sure?

hy 0.13.0+211.g5c34296 using CPython(default) 3.6.3 on Linux => (setv slice "foo") => slice 'foo'

Anyway, if we want to make get recognize a magic symbol, why not use : rather than slice? So you'd write e.g. (get foo (: 3 4)) to mean foo[3:4]. This is concise, and literal keywords are already magical as function arguments.

I do like this better. My concern was that : is HyKeyword(":"), but there's less room for things to blow up.

Are you sure?

Yes your right.

I think I was playing @builds("slice") at the time I tested it and forgot that I had put that in there.

Kodiologist · 2018-02-05T21:35:53Z

+
+        # How the subscript is constructed will depend on if any of
+        # the components subclass ast,slice. If they do, then we have
+        # to construct an ExtSlice instead of a Tuple.


Is there any reason not to always use ExtSlice instead of a tuple? That would simplify things a bit, if we could use ExtSlice for any multidimensional index.

Not sure, but I suspect its fine. I figured it made the most send to match exactly what Python does.

Oh, to expand on this, its probably worth at least merging the identical behaviour. If a simplification works, that's awesome, but that can be added next and at least there's always something more technically correct to revert to.

Kodiologist · 2018-02-05T21:38:22Z


+(defmacro cut [node &rest body]
+  "Take a subset of a list and create a new list from it."
+  `(get ~node (__builtin__slice ~@body)))


__builtins__.slice

Kodiologist · 2018-02-05T21:40:45Z

+        if symbol == "...":
+            if not PY3:
+                raise HyTypeError(symbol, 'ellipsis is only supported in '
+                                          'slices in Python 2')


The error message might be better worded as "in Python 2, ellipsis is only supported in slices".

Kodiologist · 2018-02-05T22:15:48Z

the following equivalent Python:
foo[1][2][3:4][...][5:6]
foo[(1), (2), 3:4, (...), 5:6]

These don't seem to be equivalent:

import numpy as np

foo = np.ones((7, 7, 7,  2, 2, 7))

print(foo[(1), (2), 3:4, (...), 5:6])
# prints: [[[[ 1.]
            [ 1.]]

           [[ 1.]
            [ 1.]]]]
print(foo[1][2][3:4][...][5:6])
# prints: []

vodik · 2018-02-06T18:36:22Z

I think you misunderstood - I'm not saying those two lines of Python are equivalent. Let me rephrase, and remove some noise from that complicated expression:

(get foo 1 2)               ;; foo[1][2]
(get foo (, 1 2))           ;; foo[1, 2]
(get foo (, 1 2) (, 3 4))   ;; foo[1, 2][3, 4]

There is now support for multidimensional slices while also preserving chaining. I do it by destructing the tuple - which should be safe __getitem__ interface will present it as a tuple in the first place.

Refactor to add a proper slice ast compiler. Look for "slice" and handle them specially inside subscripts to emit python slice ast.

vodik · 2018-02-08T03:48:34Z

Is this more to your taste then?

Kodiologist · 2018-02-08T06:14:27Z

Yes, it is. I'll take a closer look tomorrow.

Closes hylang#1492

Kodiologist

Finally, don't forget to document : and .... Otherwise, this PR is looking good.

Kodiologist · 2018-02-08T22:34:24Z

   * new `doc` macro and `#doc` tag macro
   * support for PEP 492 with `fn/a`, `defn/a`, `with/a` and `for/a`
   * remove `def`
+   * optimize `slice` inside subscriptions to generate slice AST


Update this line for the change to :.

Kodiologist · 2018-02-08T22:35:26Z

+  (get (CutTest) (: 1 6)))
+
+
 (defn test-cut []


Shouldn't test-cut be removed from native_tests/langauge.hy now that it's in native_tests/native_macros.hy?

Kodiologist · 2018-02-08T22:36:52Z

    @checkargs(min=2)
-    def compile_index_expression(self, expr):
-        expr.pop(0)  # index
+    def compile_index_expression(self, exprs):


The reason we call this sort of parameter expr rather than exprs is because it's set to a single HyExpression.

Kodiologist · 2018-02-08T22:38:51Z

+        if isinstance(other, ast.slice):
+            raise TypeError("Can't add %r with slice specific syntax %r" % (
+                self, other))
+


I get how the nature of ast.slice really bit you during the development of this PR, but will this check really be useful for later programmers?

Kodiologist · 2018-02-08T22:40:57Z

+        ret = Result()
+
+        for expr in exprs:
+            if expr_startswith(HyKeyword(":"), expr):


Unfortunately, this check isn't strict enough (it allows (get [1 2 3 4 5] ("\ufdd0:" 1 2))) because equality of Hy model objects is very loose about type (#1363). You probably need to check the type of expr explicitly.

Would it maybe make sense to have a crack at #1363 and fix that first?

You can try, but it's a deep rabbit hole to go down because the compiler currently depends on that "feature" in certain ways. Changes to that may also conflict with #1458. I'd advise waiting for #1458 before you do that, and finishing this first in any case.

I can wait, honestly. I'd rather see it some in on a more solid foundation.

vodik · 2018-02-08T23:01:40Z

I won't be able to get this done till tomorrow, but I'm thinking of adding some examples to the documentation as well citing numpy.

gilch · 2018-04-01T04:36:04Z

By default, when in the main module, builtins is the built-in module builtins; when in any other module, builtins is an alias for the dictionary of the builtins module itself.

Oh shoot, that's a feature. Since we hope to be setting main appropriately someday, we can't safely use get nor attribute lookup. I'm afraid I'm not sure what design would make sense, then.

At least in Python3, __import__("builtins").slice should always work, whether or not you're in __main__. Unfortunately, Python2's equivalent module is called __builtin__. But we could explicitly dump the builtins we need into the hy module or something, then it would be __import__("hy").slice in both 2 and 3. We're already normalizing some of the differences in Hy core, e.g. importing izip and renaming it zip. In this case, we'd conditionally import slice from builtins into hy when on Python 3 and from __builtin__ when on Python 2.

Kodiologist · 2018-04-25T17:17:42Z

@vodik, how are we doing here?

Kodiologist · 2019-02-28T15:47:03Z

Closed for lack of response.

vodik force-pushed the slice-opimization branch from da2c263 to 95e31ba Compare January 30, 2018 21:54

Kodiologist reviewed Jan 31, 2018

View reviewed changes

Kodiologist changed the title ~~[RFC] Proof of concept~~ [RFC] Proof of concept for slice optimization Jan 31, 2018

vodik force-pushed the slice-opimization branch from 95e31ba to 99be9be Compare January 31, 2018 04:28

vodik force-pushed the slice-opimization branch 3 times, most recently from 6fa7486 to 404c0c8 Compare February 4, 2018 23:03

vodik force-pushed the slice-opimization branch 2 times, most recently from 93b4855 to 9e38081 Compare February 4, 2018 23:45

vodik changed the title ~~[RFC] Proof of concept for slice optimization~~ Better slicing with ellipsis support Feb 4, 2018

vodik force-pushed the slice-opimization branch 4 times, most recently from 7671dd0 to 12aa09b Compare February 5, 2018 02:17

Kodiologist reviewed Feb 5, 2018

View reviewed changes

vodik added 2 commits February 7, 2018 22:29

Raise a helpful error for using slice only syntax in expressions

1f5209e

Optimize slicing by emitting ast.Slice inside ast.Subscript

aaebb2a

Refactor to add a proper slice ast compiler. Look for "slice" and handle them specially inside subscripts to emit python slice ast.

vodik force-pushed the slice-opimization branch from 5c34296 to e8799b6 Compare February 8, 2018 03:48

vodik force-pushed the slice-opimization branch from e8799b6 to 6508066 Compare February 8, 2018 03:49

Add support for Ellipsis

242bbd4

vodik force-pushed the slice-opimization branch from 6508066 to 7a456a5 Compare February 8, 2018 03:50

Add support to properly generate ExtSlice for multidimentional slices

d0e0289

Closes hylang#1492

vodik force-pushed the slice-opimization branch from 7a456a5 to d0e0289 Compare February 8, 2018 07:46

Kodiologist requested changes Feb 8, 2018

View reviewed changes

Kodiologist closed this Feb 28, 2019

Conversation

vodik commented Jan 30, 2018

Uh oh!

vodik commented Jan 30, 2018

Uh oh!

gilch commented Jan 30, 2018

Uh oh!

vodik commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kodiologist commented Jan 31, 2018

Uh oh!

Kodiologist commented Jan 31, 2018

Uh oh!

vodik commented Jan 31, 2018

Uh oh!

Kodiologist commented Jan 31, 2018

Uh oh!

vodik commented Jan 31, 2018

Uh oh!

Kodiologist commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kodiologist commented Jan 31, 2018

Uh oh!

vodik commented Feb 4, 2018

Uh oh!

Kodiologist commented Feb 4, 2018

Uh oh!

vodik commented Feb 4, 2018

Uh oh!

vodik commented Feb 4, 2018

Uh oh!

vodik commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kodiologist commented Feb 5, 2018

Uh oh!

vodik commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kodiologist Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vodik Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vodik Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kodiologist Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vodik commented Jan 30, 2018 •

edited

Loading

Kodiologist commented Jan 31, 2018 •

edited

Loading

vodik commented Feb 4, 2018 •

edited

Loading

vodik commented Feb 5, 2018 •

edited

Loading

Kodiologist Feb 5, 2018 •

edited

Loading

vodik Feb 5, 2018 •

edited

Loading

vodik Feb 5, 2018 •

edited

Loading

Kodiologist Feb 5, 2018 •

edited

Loading

vodik commented Feb 6, 2018 •

edited

Loading

Kodiologist Feb 8, 2018 •

edited

Loading

Kodiologist Feb 8, 2018 •

edited

Loading

Kodiologist Feb 9, 2018 •

edited

Loading

gilch commented Apr 1, 2018 •

edited

Loading