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

PythonCodePrinter #12808

Merged
merged 13 commits into from Jul 25, 2017

Conversation

Projects
None yet
3 participants
@bjodah
Member

bjodah commented Jun 25, 2017

To address #12213

Do we want the deprecation in before 1.1?

cc: @moorepants @asmeurer

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 25, 2017

Member

I'm typically a deprecation nazi but I doubt anyone is using the PythonPrinter because it doesn't really do anything useful. I'm not seeing anything here on first pass:

https://github.com/search?utf8=%E2%9C%93&q=PythonPrinter+sympy+language%3APython+extension%3Apy&type=Code&ref=advsearch&l=Python&l=

I wish I could exclude python.py and test_pickling.py from the search results.

I could be convinced to not deprecate this and just change it.

Member

moorepants commented Jun 25, 2017

I'm typically a deprecation nazi but I doubt anyone is using the PythonPrinter because it doesn't really do anything useful. I'm not seeing anything here on first pass:

https://github.com/search?utf8=%E2%9C%93&q=PythonPrinter+sympy+language%3APython+extension%3Apy&type=Code&ref=advsearch&l=Python&l=

I wish I could exclude python.py and test_pickling.py from the search results.

I could be convinced to not deprecate this and just change it.

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 25, 2017

Member

If I search for filenames test_pickling.py and python.py I get 267 results. If I search for any filename with the same search query I get 269 results. So I think only 2 files on github import PythonPrinter that aren't forks of SymPy.

Update:

Here's one (not even an import): https://github.com/ruoyu0088/cooknotebook/blob/054d07452a48c931639e503b59401d223ba35eab/src/sympyhelp.py

Member

moorepants commented Jun 25, 2017

If I search for filenames test_pickling.py and python.py I get 267 results. If I search for any filename with the same search query I get 269 results. So I think only 2 files on github import PythonPrinter that aren't forks of SymPy.

Update:

Here's one (not even an import): https://github.com/ruoyu0088/cooknotebook/blob/054d07452a48c931639e503b59401d223ba35eab/src/sympyhelp.py

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 25, 2017

Member

Should we move PythonPrinter to SymPyPrinter?

Update

Seems you did that.

Member

moorepants commented Jun 25, 2017

Should we move PythonPrinter to SymPyPrinter?

Update

Seems you did that.

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 25, 2017

Member

One issue is that you can also do "from sympy import python" to get this printer also. So it could be used more than my previous searches suggest.

Member

moorepants commented Jun 25, 2017

One issue is that you can also do "from sympy import python" to get this printer also. So it could be used more than my previous searches suggest.

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jun 25, 2017

Member

@moorepants, what do you mean by:

Should we move PythonPrinter to SymPyPrinter?

Member

bjodah commented Jun 25, 2017

@moorepants, what do you mean by:

Should we move PythonPrinter to SymPyPrinter?

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 25, 2017

Member

I just meant that the current PythonPrinter seems more like a SymPy printer. But it seems you realized that too and renamed it already.

Member

moorepants commented Jun 25, 2017

I just meant that the current PythonPrinter seems more like a SymPy printer. But it seems you realized that too and renamed it already.

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 25, 2017

Member

One recommendation here: break this into three PRs: PythonPrinter, NumPyPrinter, and SciPyPrinter so that it will be easier to merge them. You can likely get at least the PythonPrinter one in before 1.0.

Member

moorepants commented Jun 25, 2017

One recommendation here: break this into three PRs: PythonPrinter, NumPyPrinter, and SciPyPrinter so that it will be easier to merge them. You can likely get at least the PythonPrinter one in before 1.0.

Show outdated Hide outdated sympy/printing/python.py
@deprecated(
last_supported_version='1.2',
useinstead="SymPyPrinter",

This comment has been minimized.

@asmeurer

asmeurer Jun 25, 2017

Member

Replacement should be sympy_code, not sympy_printer. Also, should use the full path name since it isn't imported by default (sympy.printing.sympycode.sympy_code).

@asmeurer

asmeurer Jun 25, 2017

Member

Replacement should be sympy_code, not sympy_printer. Also, should use the full path name since it isn't imported by default (sympy.printing.sympycode.sympy_code).

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Jun 25, 2017

Member

I never really got the point of python(), since sstr and srepr both do the same thing. I think perhaps it predates them. At any rate, it is in the global namespace, so it should be deprecated if it is going to be removed.

Member

asmeurer commented Jun 25, 2017

I never really got the point of python(), since sstr and srepr both do the same thing. I think perhaps it predates them. At any rate, it is in the global namespace, so it should be deprecated if it is going to be removed.

Show outdated Hide outdated sympy/printing/pycode.py
'with', 'yield'
}
_kw_only_py2 = {'exec', 'print'}
_kw_only_py3 = {'False', 'None', 'nonlocal', 'True'}

This comment has been minimized.

@asmeurer

asmeurer Jun 25, 2017

Member

None is sort of a keyword in Python 2, since you cannot assign to it. I'm assuming that is how reserved_words is used.

@asmeurer

asmeurer Jun 25, 2017

Member

None is sort of a keyword in Python 2, since you cannot assign to it. I'm assuming that is how reserved_words is used.

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jun 27, 2017

Member

I have been thinking about this. Just introducing PythonCodePrinter does not add much value IMHO unless we make use of new AST nodes in .codegen.ast (e.g. "FunctionDefinition", "IfStatement" etc.). So that would take quite a bit of work (=> no chance of getting it in before v1.1). But if we want a rename and depreaction of python in I could open a PR for that? (and I agree with @asmeurer―I don't see the point of even keeping the python function)

Member

bjodah commented Jun 27, 2017

I have been thinking about this. Just introducing PythonCodePrinter does not add much value IMHO unless we make use of new AST nodes in .codegen.ast (e.g. "FunctionDefinition", "IfStatement" etc.). So that would take quite a bit of work (=> no chance of getting it in before v1.1). But if we want a rename and depreaction of python in I could open a PR for that? (and I agree with @asmeurer―I don't see the point of even keeping the python function)

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 27, 2017

Member

What is the reason not to use the current code printer as a subclass for the new PythonCodePrinter? Wouldn't that at least work for now? And then once the ast code is done you can modify all the CodePrinters to use it?

Member

moorepants commented Jun 27, 2017

What is the reason not to use the current code printer as a subclass for the new PythonCodePrinter? Wouldn't that at least work for now? And then once the ast code is done you can modify all the CodePrinters to use it?

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Jun 27, 2017

Member

If we do that maybe we should actually keep the python name. That would require no deprecation.

Member

asmeurer commented Jun 27, 2017

If we do that maybe we should actually keep the python name. That would require no deprecation.

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jun 27, 2017

Member

@moorepants I suppose. So then the new PythonCodePrinter printer will essentially fix:

>>> python_code(Mod(x, 2))
x % 2

(from the original issue) since lambdarepr currently prints:

>>> lambdarepr(Mod(x, 2))
'Mod(x, 2)'

looking at the "And" example from the original issue:

>>> lambdarepr(And(x, y, evaluate=False))
'((x) and (y))'
>>>> print(python(And(x, y, evaluate=False)))
x = Symbol('x')
y = Symbol('y')
e = x & y

so boolean and bitwise "and" from those printers... (I would consider using bitwise "or" a bug if it wasn't for the symbols being defined in the same block of code).

So then it's a matter of fixing small things wrt. lambdarepr (shouldn't it even output % to begin with?)
Maybe we should discuss what the goal is with this printer?

Member

bjodah commented Jun 27, 2017

@moorepants I suppose. So then the new PythonCodePrinter printer will essentially fix:

>>> python_code(Mod(x, 2))
x % 2

(from the original issue) since lambdarepr currently prints:

>>> lambdarepr(Mod(x, 2))
'Mod(x, 2)'

looking at the "And" example from the original issue:

>>> lambdarepr(And(x, y, evaluate=False))
'((x) and (y))'
>>>> print(python(And(x, y, evaluate=False)))
x = Symbol('x')
y = Symbol('y')
e = x & y

so boolean and bitwise "and" from those printers... (I would consider using bitwise "or" a bug if it wasn't for the symbols being defined in the same block of code).

So then it's a matter of fixing small things wrt. lambdarepr (shouldn't it even output % to begin with?)
Maybe we should discuss what the goal is with this printer?

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 27, 2017

Member

Could small things in lambdarepr be fixed and then a class created PythonCodePrinter that subclasses it (or move all code from LambdaPrinter to PythonCodePrinter)? It just isn't obvious to look for something called lambda if you want to print Python code.

Member

moorepants commented Jun 27, 2017

Could small things in lambdarepr be fixed and then a class created PythonCodePrinter that subclasses it (or move all code from LambdaPrinter to PythonCodePrinter)? It just isn't obvious to look for something called lambda if you want to print Python code.

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jun 27, 2017

Member

I completely agree that the naming is suboptimal (we need to decide on python as well―I would like to rename it, but deprecation is always a hassle, so I can see if we just want to leave it be).

I suppose I could fix things in lambdarepr. Do we have a list (apart from Mod)?

Member

bjodah commented Jun 27, 2017

I completely agree that the naming is suboptimal (we need to decide on python as well―I would like to rename it, but deprecation is always a hassle, so I can see if we just want to leave it be).

I suppose I could fix things in lambdarepr. Do we have a list (apart from Mod)?

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 27, 2017

Member

Would it be possible to have both LambdaReprPrinter and PythonCodePrinter and they are essentially mirrors of each other? Then in the docs we just show of PythonCodePrinter and don't show anything about the former, i.e. keep in undocumented of sorts.

Member

moorepants commented Jun 27, 2017

Would it be possible to have both LambdaReprPrinter and PythonCodePrinter and they are essentially mirrors of each other? Then in the docs we just show of PythonCodePrinter and don't show anything about the former, i.e. keep in undocumented of sorts.

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Jun 28, 2017

Member

I think lambdarepr should print expressions only, but the python printer can print any block of Python code. Piecewise is another example. We could print it with real if/elif/else chains instead of nested ternary statements.

Member

asmeurer commented Jun 28, 2017

I think lambdarepr should print expressions only, but the python printer can print any block of Python code. Piecewise is another example. We could print it with real if/elif/else chains instead of nested ternary statements.

@moorepants

This comment has been minimized.

Show comment
Hide comment
Member

moorepants commented Jun 28, 2017

Agreed @asmeurer

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jun 28, 2017

Member

This will not be a small change, tons of subtleties (StrPrinter vs. CodePrinter), we can see how it goes.

Member

bjodah commented Jun 28, 2017

This will not be a small change, tons of subtleties (StrPrinter vs. CodePrinter), we can see how it goes.

@bjodah bjodah changed the title from [WIP] Python-, NumPy- & SciPy- CodePrinter:s to [WIP] PythonCodePrinter Jun 30, 2017

@bjodah bjodah changed the title from [WIP] PythonCodePrinter to PythonCodePrinter Jul 24, 2017

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jul 24, 2017

Member

I think this might be ready. Do we want PythonCodePrinter to do anything more?

Member

bjodah commented Jul 24, 2017

I think this might be ready. Do we want PythonCodePrinter to do anything more?

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Jul 24, 2017

Member

The end goal of this is to move the default translations from lambdify.py into the printers. Should we do that here or in another PR?

Member

asmeurer commented Jul 24, 2017

The end goal of this is to move the default translations from lambdify.py into the printers. Should we do that here or in another PR?

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jul 24, 2017

Member

We could do it either way. I'm thinking review is simpler doing multiple PRs?

Member

bjodah commented Jul 24, 2017

We could do it either way. I'm thinking review is simpler doing multiple PRs?

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Jul 24, 2017

Member

So I think this looks good as is. Feel free to merge if you want.

Member

asmeurer commented Jul 24, 2017

So I think this looks good as is. Feel free to merge if you want.

@bjodah bjodah merged commit a196c42 into sympy:master Jul 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bjodah bjodah deleted the bjodah:pythonprinter branch Jul 25, 2017

@bjodah bjodah referenced this pull request Aug 14, 2017

Closed

Python code printer #12213

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Apr 29, 2018

Member

Seeing some performance regressions from this PR, see #14671.

Member

moorepants commented Apr 29, 2018

Seeing some performance regressions from this PR, see #14671.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment