On Saturday, 25 January 2014 20:13:12 UTC+1, Aaron Meurer wrote:
>
> On Sat, Jan 25, 2014 at 1:05 PM, Björn Dahlgren
> <bjo...@gmail.com<javascript:>>
> wrote:
> > On Saturday, 25 January 2014 19:55:58 UTC+1, Aaron Meurer wrote:
> >>
> >> Well in theory it's easy. You just need to edit the exclude part of
> >> the iterable definition in sympy/core/compatiblity.py to include
> >> IndexedBase. But this is actually not so easy because there's no way
> >> to import IndexedBase for use in the function, since it is used in the
> >> import process before IndexedBase is defined.
> >
> >
> > Yeah I ran into that problem, but iterable takes `exclude` kwarg so I
> tried:
> > iterable(expr, exclude=(string_types, dict, IndexedBase))
>
> Oh sure, you can exclude it in that once place. But we really should
> make this work globally.
>
> >
> > based on what you said about IndexedBase needing to be added to the list
> >
> > then there is no infinite loop anymore but the output is not what one
> would
> > expect:
> >
> > ([(x0, y), (x1, i), (x2, x1 + 1), (x3, x), (x4, (-x3[x1] +
> x3[x2])**(-1))],
> > [x4*(-x0[x1] + x0[x2]), x4])
> >
> > so it is actually looking for cse's in indices which is maybe a bit
> > overkill..
>
> Well, that's a separate issue. All cse() does is look at the .args of
> the expressions. Technically i + 1 is a common subexpression (and I
> suppose one could in theory have a more complicated common
> subexpression in the index).
>
Yes, giving this a second thought I think you are right. In that respect the
PR in it's current state should pretty much cover this use case (the test
is not quite precise enough though..)
>
> >
> > But apart from that it acutally works!
> >
> >
> >>
> >>
> >> So probably what should be done is some kind of _not_iterable property
> >> put on IndexedBase and iterable() changed to look for it.
> >>
> >> By the way, I think there are other objects that do the same sort of
> >> thing, like MatrixSymbol, which also need to be disabled, since they
> >> iterate forever. I seem to remember a few places in the code just
> >> check for them explicitly, but we should standardize on iterable(),
> >> and make iterable() know what does and doesn't work.
> >
> >
> > Makes sense, I don't nearly have the overview to know what should be
> > excluded
> > (and if there are exceptions for certain applications). So maybe passing
> > modified
> > exclude kwarg is a good workaround for now? Making it not look into the
> > indices would
> > be nice though.
>
> I think IndexedBase and MatrixExpr are the only objects that do this.
> Maybe something in the tensor module too. It should be a simple fix.
> Just add some property, like
>
> _iterable = False
>
> to the class, and then modify iterable() to look for that property.
>
> I suppose you could also use a mixin, like
>
> class NotIterable(object):
> pass
>
> and add that so the subclasses of IndexedBase and MatrixExpr, and to
> default the exclude tuple.
>
The latter solution looks good to me but I don't know what you guys prefer
for consistency throughout the code base.
So I think it's a design decision which I'm not the right person the make.
I could modify my PR as per your request
though.
>
> Aaron Meurer
>
> >
> >
> >>
> >>
> >> Aaron Meurer
> >>
> >>
> >> On Sat, Jan 25, 2014 at 12:45 PM, Björn Dahlgren <bjo...@gmail.com>
> wrote:
> >> > Ok, I poked around a little didn't get it to work on my first try.
> >> > Do you agree that it would be the preferred behaviour that cse should
> >> > work
> >> > with Indexed instances out of the box?
> >> > Is it trivial fix for someone - if not I could try to get this to
> work
> >> > but
> >> > it will take me quite a bit of work I think.
> >> >
> >> > Thanks!
> >> > /Björn
> >> >
> >> >
> >> > On Saturday, 25 January 2014 19:26:47 UTC+1, Aaron Meurer wrote:
> >> >>
> >> >> The problem is that the iterable() function in compatibility thinks
> >> >> that y is iterable, so it's trying to iterate through y. The fix is
> to
> >> >> add IndexedBase to the set of ignored iterables in the function.
> >> >>
> >> >> Aaron Meurer
> >> >>
> >> >> On Sat, Jan 25, 2014 at 5:04 AM, Björn Dahlgren <bjo...@gmail.com>
> >> >> wrote:
> >> >> > Hi!
> >> >> >
> >> >> > while using the Indexed class from sympy.tensor I have run into
> the
> >> >> > following problem:
> >> >> >
> >> >> > cse(...) fails for expressions having "Indexed" instances. They
> are
> >> >> > not
> >> >> > handled correctly in opt_cse() in cse_main.py
> >> >> > A possible work-around is to substitute the Indexed instances with
> >> >> > dummies,
> >> >> > perform the CSE elimination and then
> >> >> > resubstitute the dummies for the indexed symbols. But this is not
> >> >> > pretty
> >> >> > and
> >> >> > I think that cse(...) should handle this
> >> >> > on its own.
> >> >> >
> >> >> > Example:
> >> >> >
> >> >> > from sympy import *
> >> >> >
> >> >> > len_y = 5
> >> >> > y = IndexedBase('y', shape=(len_y,))
> >> >> > x = IndexedBase('x', shape=(len_y,))
> >> >> > Dy = IndexedBase('Dy', shape=(len_y-1,))
> >> >> > i = Idx('i', len_y-1)
> >> >> >
> >> >> > expr1 = (y[i+1]-y[i])/(x[i+1]-x[i])
> >> >> > expr2 = 1/(x[i+1]-x[i])
> >> >> > print(cse([expr1, expr2]))
> >> >> >
> >> >> > this will cause an infinite recursive call stack to _find_opts
> >> >> > raising:
> >> >> > RuntimeError: maximum recursion depth exceeded in
> >> >> > __instancecheck__
> >> >> >
> >> >> >
> >> >> > Does anybody know what the best approach here would be? Add
> something
> >> >> > to
> >> >> > the
> >> >> > Indexed class or modify
> >> >> > opt_cse?
> >> >> >
> >> >> > Best regards
> >> >> > /Björn
> >> >> >
> >> >> > --
> >> >> > You received this message because you are subscribed to the Google
> >> >> > Groups
> >> >> > "sympy" group.
> >> >> > To unsubscribe from this group and stop receiving emails from it,
> >> >> > send
> >> >> > an
> >> >> > email to sympy+un...@googlegroups.com.
> >> >> > To post to this group, send email to sy...@googlegroups.com.
> >> >> > Visit this group at http://groups.google.com/group/sympy.
> >> >> > For more options, visit https://groups.google.com/groups/opt_out.
> >> >
> >> > --
> >> > You received this message because you are subscribed to the Google
> >> > Groups
> >> > "sympy" group.
> >> > To unsubscribe from this group and stop receiving emails from it,
> send
> >> > an
> >> > email to sympy+un...@googlegroups.com.
> >> > To post to this group, send email to sy...@googlegroups.com.
> >> > Visit this group at http://groups.google.com/group/sympy.
> >> > For more options, visit https://groups.google.com/groups/opt_out.
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups
> > "sympy" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an
> > email to sympy+un...@googlegroups.com <javascript:>.
> > To post to this group, send email to sy...@googlegroups.com<javascript:>.
>
> > Visit this group at http://groups.google.com/group/sympy.
> > For more options, visit https://groups.google.com/groups/opt_out.
>
--
You received this message because you are subscribed to the Google Groups
"sympy" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to sympy+unsubscr...@googlegroups.com.
To post to this group, send email to sympy@googlegroups.com.
Visit this group at http://groups.google.com/group/sympy.
For more options, visit https://groups.google.com/groups/opt_out.
