#12834: Modify subs so that it can accept multiple equations just like subs_expr
-------------------------------------+-------------------------------------
Reporter: JoalHeagney | Owner: AlexGhitza
Type: enhancement | Status: needs_work
Priority: minor | Milestone: sage-6.4
Component: symbolics | Resolution:
Keywords: subs algebra | Merged in:
solving | Reviewers: Vincent Delecroix,
Authors: Michael Orlitzky, | Michael Orlitzky
Vincent Delecroix | Work issues:
Report Upstream: N/A | Commit:
Branch: | 24b98a061c140e9f76b171eb3dd1be0fa44d36b8
u/mjo/ticket/12834 | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Comment (by mjo):
Replying to [comment:23 vdelecroix]:
> Hello,
>
> I do not agree with your `05492f4` and `0d34382`. First of all, if
something is wrong, then it is not a big deal to go through the
dictionaries again. Your version is way much too complicated. Please do
something along
> {{{
> dup = [k for k in d2 if k in d1]
> if dup:
> k = min(dup)
> msg = "duplicate substitution for {}, got values {} and {}"
> raise ValueError(msg.format(k, d1[k], d2[k]))
> }}}
> And you can remark that I iterated over `d2` and this was intentional.
The dictionary `d1` is intended to be large compared to `d2` (think about
`expr.subs(u == 18, v == 15, w == 19, x == 1, y == 2, z == 3)`). In your
commit you reversed that. And you should know that to get the minimum of a
list you do not need to sort it ;-) Was the call to `sorted` intentional
compared to `min`?
I guess I wasn't very clear in my commit message =)
The original code was,
{{{
if any(k in d1 for k in d2):
k = (k for k in d1 if k in d2).next()
raise ValueError...
}}}
The first `any(k in d1 for k in d2)` is probably O(m*n), since it
(potentially) has to look through all of both dictionaries to see if there
are any duplicates. Then,
{{{
k = (k for k in d1 if k in d2).next()
}}}
'''also''' looks through both dictionaries, essentially to do the same
thing: to see if there are any duplicates, but this time we write down the
first duplicate key.
Since we're going to find the first duplicate key ''anyway'', we don't
need to do the first `any` check. If the dupe is toward the end of the
list, the `any`less version could be a lot faster. Once the `any` check is
gone, my version isn't much different than yours. But since I've removed
the `any` check, I no longer know that `next()` is safe in
{{{
k = (k for k in d1 if k in d2).next()
}}}
because it could throw a `StopIteration` exception. Instead of catching
the exception, I chose to use a "for ... in" loop iteration:
{{{
for k in (k for k in d1 if k in d2)
}}}
That achieves the same thing without having to check whether or not there
were any dupes. I tested it with some big dictionaries and it is a little
faster. But since I can't force a dupe to be "at the end" of a dictionary,
I can't check the worst-case.
In any case, I'm going to push another version that should be faster and
simpler than both =)
--
Ticket URL: <http://trac.sagemath.org/ticket/12834#comment:26>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
