-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor implicit parsing #1871
Conversation
SymPy Bot Summary: ✳️ Passed after merging lidavidm/refactor_implicit_parsing (c966c6a) into master (9bc7091). |
Was this originally added before or after the 0.7.2 release? If after, go ahead and break the API without abandon. And even if before, we should consider this module to be still in a development stage, and hence should probably consider the API to be evolving. |
symbol = tok[1][1:-1] | ||
if _token_splittable(symbol): | ||
for char in symbol: | ||
result.extend([(NAME, "'{}'".format(char)), (OP, ')'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Causes failures in python 2.5 and 2.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess just use +=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh never mind, it's the format, not the extend. Just use %s.
I'm assuming the old tests are still testing the same behavior, but at least add tests that the functions are split correctly. |
Let's refactor |
It looks like people are already using this functionality. See http://colabti.org/irclogger/irclogger_log/sympy?date=2013-03-06#l166. It's probably because I mentioned it on StackOverflow. |
Changes:
|
SymPy Bot Summary: ✳️ Passed after merging lidavidm/refactor_implicit_parsing (a48a129) into master (6f0e757). |
Are any other changes needed? |
I mean, test things like |
Add tests for the custom splitting. |
Changes:
|
SymPy Bot Summary: ✳️ Passed after merging lidavidm/refactor_implicit_parsing (5cae26e) into master (07933cd). |
This looks good. For the API, we should think of a more extensible way to do this. It is probably more elegant to have an object oriented solution, where you override methods on the base class to get custom behavior (c.f. the way that the ast module works in the standard library). |
Also http://greentreesnakes.readthedocs.org/en/latest/ might be a useful read if you find the API docs for ast too dense for a first read. |
But as it were, this is fine, so I'm merging. If there are any functional changes here we can update SymPy Gamma (are there?). |
This shouldn't affect Gamma. In any case, I can update Gamma in the next pull I'm working on (steps for derivatives and maybe some integrals). For the API, you're suggesting some sort of Visitor/Transformer class (but for tokens) akin to the AST module? |
Or maybe just use AST, instead of tokenize. I don't know if it will be literally "visitor" and "transformer". My point is that APIs where you subclass and override what you what to change work quite well for these things. I can point to other completely unrelated things that work this way too. For example, HTMLParser in the standard library. Doing things this way lets you only define those things that are really different for your case. The rest will just come from inheritance. And if you understand how to properly apply it, you can get some pretty good mileage out of the method resolution order and multiple inheritance. If it's not immediately obvious to you how to design this (and it probably isn't), I would recommend asking on the list and seeing if the community can develop a good API for it. |
Okay, I'll look into it. |
Changes:
sin 2x
only works if implicit multiplication comes before implicit application.sin 2x
works now. (Before it gavex*sin(2)
, now it givessin(2*x)
.)https://code.google.com/p/sympy/issues/detail?id=3678