Skip to content
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

Improve 'simplify' function #22

Merged
merged 1 commit into from
Oct 13, 2013
Merged

Conversation

magistere
Copy link
Contributor

No description provided.

@johnmyleswhite
Copy link
Collaborator

Very nice. Thanks!

johnmyleswhite added a commit that referenced this pull request Oct 13, 2013
Improve 'simplify' function
@johnmyleswhite johnmyleswhite merged commit 903e80d into JuliaMath:master Oct 13, 2013
@ivarne
Copy link
Collaborator

ivarne commented Oct 13, 2013

I tried to implement this functionality some months ago, and I think I remember that the multiplication was more complex because the parser translate multiplication to a chain of binary operations. I will check tomorrow.

@johnmyleswhite
Copy link
Collaborator

That may be true. But that shouldn't break this code, just make it superfluous, right?

@ivarne
Copy link
Collaborator

ivarne commented Oct 14, 2013

Hmm.. It looks like the issue I was having has been fixed. I tried to get some comments on my own attempt at fixing simplify! in #14 (comment), but apparently nobody noticed.

@magistere magistere deleted the simplify branch October 14, 2013 06:18
@magistere
Copy link
Contributor Author

@ivarne I assume that something was changed in multiplication implementation in Julia core. Now I get

julia> simplify(:(*x))
ERROR: syntax: missing separator in tuple

So, the first special case in simplify(::SymbolParameter{:*}, args) should be modified or removed.

@ivarne
Copy link
Collaborator

ivarne commented Oct 14, 2013

That is just a syntax issue, you can still do :(*(x)) and get to the special case. It is pretty useless, but still a valid simplification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants