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

Further work on PythonCodePrinter #13046

Merged
merged 18 commits into from Aug 9, 2017

Conversation

Projects
None yet
3 participants
@bjodah
Member

bjodah commented Jul 26, 2017

Two goals with this PR:

  • Base NumPy- and Mpmath-printers off PythonCodePrinter
  • Refactor lambdify not to use TRANSLATIONS dicts (and instead use _print_XXX methods which may be overloaded in subclasses).

TODO

  • get rid of wrapper class inside Lambdify
@@ -424,6 +454,13 @@ def lambdify(args, expr, modules=None, printer=None, use_imps=True,
# Create lambda function.
lstr = lambdastr(args, expr, printer=printer, dummify=dummify)
flat = '__flatten_args__'
imp_mod_lines = []
for mod, keys in (getattr(printer, 'module_imports', None) or {}).items():

This comment has been minimized.

@asmeurer

asmeurer Aug 1, 2017

Member

Could there be duplicate names? If so, the order will matter here.

@asmeurer

asmeurer Aug 1, 2017

Member

Could there be duplicate names? If so, the order will matter here.

This comment has been minimized.

@bjodah

bjodah Aug 2, 2017

Member

That would be a problem in the printer already then (unlikely and would require two different _print methods to output functions with same name from different modules). The order in the modules kwarg to lambdify does still matter though.

@bjodah

bjodah Aug 2, 2017

Member

That would be a problem in the printer already then (unlikely and would require two different _print methods to output functions with same name from different modules). The order in the modules kwarg to lambdify does still matter though.

@bjodah bjodah changed the title from [WIP] Further work on PythonCodePrinter to Further work on PythonCodePrinter Aug 2, 2017

Show outdated Hide outdated sympy/utilities/lambdify.py
"math": (MATH, MATH_DEFAULT, MATH_TRANSLATIONS, ("from math import *",)),
"mpmath": (MPMATH, MPMATH_DEFAULT, MPMATH_TRANSLATIONS, ("from mpmath import *",)),
"numpy": (NUMPY, NUMPY_DEFAULT, NUMPY_TRANSLATIONS, ("import_module('numpy')",)),
"numpy": (NUMPY, {}, {}, ("import numpy",)),

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

We should probably leave these alone and functioning in case someone is using them. Or do you think it isn't worth keeping compatibility here?

@asmeurer

asmeurer Aug 2, 2017

Member

We should probably leave these alone and functioning in case someone is using them. Or do you think it isn't worth keeping compatibility here?

This comment has been minimized.

@bjodah

bjodah Aug 2, 2017

Member

People might rely on the NUMPY_TRANSLATIONS in the module (at least I do in a project). I never thought about that it might be needed by some lambdified code thats not tested. It was good that tests passed without these, but I think you're right. I will reintroduce them.

@bjodah

bjodah Aug 2, 2017

Member

People might rely on the NUMPY_TRANSLATIONS in the module (at least I do in a project). I never thought about that it might be needed by some lambdified code thats not tested. It was good that tests passed without these, but I think you're right. I will reintroduce them.

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Should we make the translation dictionaries empty by default now, since the actual default translations are done in the printers now?

@asmeurer

asmeurer Aug 2, 2017

Member

Should we make the translation dictionaries empty by default now, since the actual default translations are done in the printers now?

Show outdated Hide outdated sympy/utilities/lambdify.py
else:
reg = False
self._module_format((
(func.__module__ + '.') if func.__module__ else ''

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

If you have a function within a function, like

def test():
    def _test():
        pass

Then _test.__module__ will be the module of the file it is defined in, but it won't be accessible from it.

@asmeurer

asmeurer Aug 2, 2017

Member

If you have a function within a function, like

def test():
    def _test():
        pass

Then _test.__module__ will be the module of the file it is defined in, but it won't be accessible from it.

_not_in_numpy = 'erf erfc factorial gamma lgamma'.split()
_in_numpy = [(k, v) for k, v in _known_functions_math.items() if k not in _not_in_numpy]
_known_functions_numpy = dict(_in_numpy, **{

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Does this include everything from NUMPY_TRANSLATIONS?

@asmeurer

asmeurer Aug 2, 2017

Member

Does this include everything from NUMPY_TRANSLATIONS?

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

Yes, except 'ln' which I believe is wrong in the translations dict.

@bjodah

bjodah Aug 7, 2017

Member

Yes, except 'ln' which I believe is wrong in the translations dict.

Show outdated Hide outdated sympy/printing/lambdarepr.py
@@ -335,6 +205,7 @@ def _print_Function(self, e):
else:
raise TypeError("numexpr does not support function '%s'" %
func_name)
print(nstr)

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Remove this

@asmeurer

asmeurer Aug 2, 2017

Member

Remove this

@bjodah bjodah changed the title from Further work on PythonCodePrinter to [WIP] Further work on PythonCodePrinter Aug 2, 2017

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Aug 7, 2017

Member

I got rid of the Wrapper class, there is still a bit of a work-around applied (but using the less invasive approach of passing user_functions as a printer setting).

I think is ready for another round of review.

Member

bjodah commented Aug 7, 2017

I got rid of the Wrapper class, there is still a bit of a work-around applied (but using the less invasive approach of passing user_functions as a printer setting).

I think is ready for another round of review.

Show outdated Hide outdated sympy/utilities/lambdify.py
@@ -102,10 +102,10 @@
NUMEXPR_TRANSLATIONS = {}
# Available modules:
MODULES = {
_MODULES = {

This comment has been minimized.

@asmeurer

asmeurer Aug 8, 2017

Member

I think this name should be left alone for backwards compatibility purposes.

@asmeurer

asmeurer Aug 8, 2017

Member

I think this name should be left alone for backwards compatibility purposes.

This comment has been minimized.

@bjodah

bjodah Aug 8, 2017

Member

Yes, you're probably right. That dictionary is affected by calls to lambdify though so people really shouldn't be, but I'll revert the change.

@bjodah

bjodah Aug 8, 2017

Member

Yes, you're probably right. That dictionary is affected by calls to lambdify though so people really shouldn't be, but I'll revert the change.

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Aug 8, 2017

Member

Shouldn't we now clear out the default translations like NUMPY_TRANSLATIONS if they are in the printers.

Member

asmeurer commented Aug 8, 2017

Shouldn't we now clear out the default translations like NUMPY_TRANSLATIONS if they are in the printers.

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Aug 8, 2017

Member

Ummm, I thought you said the opposite here?
#13046 (comment)

(I'm probably misunderstanding)

Member

bjodah commented Aug 8, 2017

Ummm, I thought you said the opposite here?
#13046 (comment)

(I'm probably misunderstanding)

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Aug 8, 2017

Member

I was thinking "functioning" in the sense of if someone adds something to the dictionary, it will still work. I guess clearing the dictionaries would break it if someone was using the predefined values, but it seems incorrect to me to have the translations there if the printer doesn't actually use them.

Member

asmeurer commented Aug 8, 2017

I was thinking "functioning" in the sense of if someone adds something to the dictionary, it will still work. I guess clearing the dictionaries would break it if someone was using the predefined values, but it seems incorrect to me to have the translations there if the printer doesn't actually use them.

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Aug 9, 2017

Member

I see, clearing the contents would at least break one of my own codes. In this case that's obviously totally fine, I can just fix that code myself, but others might be doing the same, I've cleared the dictionary now.

Member

bjodah commented Aug 9, 2017

I see, clearing the contents would at least break one of my own codes. In this case that's obviously totally fine, I can just fix that code myself, but others might be doing the same, I've cleared the dictionary now.

@bjodah bjodah changed the title from [WIP] Further work on PythonCodePrinter to Further work on PythonCodePrinter Aug 9, 2017

@bjodah bjodah merged commit 8ff744d into sympy:master Aug 9, 2017

1 check passed

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

@bjodah bjodah deleted the bjodah:pythonprinter2 branch Aug 9, 2017

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