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

Add Type, Variable, Pointer & Declaration to .codegen.ast #12693

Merged
merged 34 commits into from Aug 10, 2017

Conversation

Projects
None yet
3 participants
@bjodah
Member

bjodah commented May 30, 2017

This is a work-in-progress PR for bringing new ast nodes into .codegen.ast.

TODO

  • Figure out correct use of _sympify with args of Basic (None does not sympfiy).
    Using nargs = (1, 2) having None as second argument is handled by only storing the first arg.
  • Figure out how strings should be handled as arguments to Basic. e.g. Symbol uses .name.
    Type now subclasses Symbol
  • Determine sane default for type inference from SymPy types to types relevant for native languages. (emphasis on machine native types handled by the FPU).
  • Implement printers for Declaration in:
    • CCodePrinter
    • FCodePrinter

Quite likely also in this PR (since it would reveal if the Type abstraction is powerful enough):

  • Implement printers for float & long double math functions (sinf, sinl & friends)

Possibly also in this PR (since it would reveal if the Declaration class is sufficiently versatile):

  • arrays:
    • contiguous 1D
    • 2D (column major, row major)
    • N-dimensional
    • strided
    • non-contiguous (e.g.: accessor macros / double pointers in C, operator() -> T& in C++)
    • compile time vs. runtime determined shapes / strides
    • constness and aliasing (const/restrict/__restrict__ in C/C++, InOut etc. in Fortran)
@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 31, 2017

Member

Do you want to add printers here or should that be done in a separate pull request?

Member

asmeurer commented May 31, 2017

Do you want to add printers here or should that be done in a separate pull request?

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah May 31, 2017

Member
Member

bjodah commented May 31, 2017

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jun 5, 2017

Member

This might actually be a good time for review, what do you think @asmeurer ?

EDIT: regarding the upcoming v1.1: I'd like to reserve the right to change these classes without prior deprecation―will you fork master soon? (or should I put these classes under some "sandbox" submodule?)

Member

bjodah commented Jun 5, 2017

This might actually be a good time for review, what do you think @asmeurer ?

EDIT: regarding the upcoming v1.1: I'd like to reserve the right to change these classes without prior deprecation―will you fork master soon? (or should I put these classes under some "sandbox" submodule?)

@asmeurer

My review so far. Will try to finish it up later today or tomorrow.

Show outdated Hide outdated sympy/codegen/ast.py
Either an explicit type: ``intc``, ``intp``, ``int8``, ``int16``,
``int32``, ``int64``, ``uint8``, ``uint16``, ``uint32``, ``uint64,
float16``, ``float32``, ``float64``, ``complex64``, ``complex128``,
``bool. Or only kind (precision decided by code-printer): ``integer``,

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

"only kind" is awkward wording.

@asmeurer

asmeurer Jun 5, 2017

Member

"only kind" is awkward wording.

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

Should also include int and float, to match the Python names.

@asmeurer

asmeurer Jun 5, 2017

Member

Should also include int and float, to match the Python names.

Show outdated Hide outdated sympy/codegen/ast.py
float16``, ``float32``, ``float64``, ``complex64``, ``complex128``,
``bool. Or only kind (precision decided by code-printer): ``integer``,
``real`` or ``complex`` (where the latter two are of floating point type).
If a ``Type`` instance is given, the said instance is returned.

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

Should probably also support the builtins int, float, complex like NumPy.

@asmeurer

asmeurer Jun 5, 2017

Member

Should probably also support the builtins int, float, complex like NumPy.

Examples
--------
>>> from sympy.codegen.ast import Type
>>> Type.from_expr(42).name

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

You should mention this classmethod in the text above.

@asmeurer

asmeurer Jun 5, 2017

Member

You should mention this classmethod in the text above.

Show outdated Hide outdated sympy/codegen/ast.py
default_limits['complex128'] = default_limits['float64']
default_precision_targets = { # e.g.:
# 'real': 'float64',

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

Is this supposed to be commented out?

@asmeurer

asmeurer Jun 5, 2017

Member

Is this supposed to be commented out?

Show outdated Hide outdated sympy/codegen/ast.py
[1] https://docs.scipy.org/doc/numpy/user/basics.types.html
"""
allowed_names = tuple('intc intp int8 int16 int32 int64 uint8 uint16 uint32'.split() +

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

This is not used.

@asmeurer

asmeurer Jun 5, 2017

Member

This is not used.

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

Maybe instead of checking in the constructor, we can just have a set of known types as constants in this file, like

intc = Type('intc')
intp = Type('intp')
...
@asmeurer

asmeurer Jun 5, 2017

Member

Maybe instead of checking in the constructor, we can just have a set of known types as constants in this file, like

intc = Type('intc')
intp = Type('intp')
...
Show outdated Hide outdated sympy/codegen/ast.py
The typename will be deduced from type or properties. Default is 'real'
(e.g. when a string is given as expr).
symb : Symbol (optional)
If given, assumptions of ``symb`` has higher precedence than expr.

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

If I am reading this right, symb is not actually used, other than for assumptions. Perhaps we could use the new assumptions here.

@asmeurer

asmeurer Jun 5, 2017

Member

If I am reading this right, symb is not actually used, other than for assumptions. Perhaps we could use the new assumptions here.

Show outdated Hide outdated sympy/codegen/ast.py
----------
expr : number, string or SymPy object
The typename will be deduced from type or properties. Default is 'real'
(e.g. when a string is given as expr).

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

This is unclear.

@asmeurer

asmeurer Jun 5, 2017

Member

This is unclear.

Show outdated Hide outdated sympy/codegen/ast.py
>>> from sympy.codegen.ast import Type
>>> Type.from_expr(2) == Type('integer')
True
>>> Type.from_expr('i') == Type('integer')

This comment has been minimized.

@asmeurer

asmeurer Jun 5, 2017

Member

This example is misleading. What is the point of allowing string inputs?

@asmeurer

asmeurer Jun 5, 2017

Member

This example is misleading. What is the point of allowing string inputs?

Show outdated Hide outdated sympy/codegen/ast.py
_tiny = lim(name, 'tiny')
dec_dig = lim(name, 'decimal_dig')
val = Float(str(val), dec_dig+3)
caster = lambda x: Float(str(x.evalf(dec_dig)), dec_dig+3)

This comment has been minimized.

@asmeurer

asmeurer Jun 7, 2017

Member

Float can accept a binary precision now. Should we use that instead?

@asmeurer

asmeurer Jun 7, 2017

Member

Float can accept a binary precision now. Should we use that instead?

Show outdated Hide outdated sympy/codegen/ast.py
raise ValueError("Maximum value exceeded for data type.")
if abs(re(val)) < _tiny or abs(im(val)) < _tiny:
raise ValueError("Minimum (absolute) value for data type bigger than new value.")
new_val = caster(re(val)) + 1j*caster(im(val))

This comment has been minimized.

@asmeurer

asmeurer Jun 7, 2017

Member

caster calls re and im, but it only gets passed real numbers. Is this correct?

@asmeurer

asmeurer Jun 7, 2017

Member

caster calls re and im, but it only gets passed real numbers. Is this correct?

This comment has been minimized.

@bjodah

bjodah Jul 3, 2017

Member

Yes, my assumption here is that there is a 1:1 correspondence between any floating point representation and its complex counter part (occupying twice the number of bits)

@bjodah

bjodah Jul 3, 2017

Member

Yes, my assumption here is that there is a 1:1 correspondence between any floating point representation and its complex counter part (occupying twice the number of bits)

This comment has been minimized.

@asmeurer

asmeurer Jul 3, 2017

Member

I mean it seems like your definition is wrong. Should it just be new_val = caster(val)?

@asmeurer

asmeurer Jul 3, 2017

Member

I mean it seems like your definition is wrong. Should it just be new_val = caster(val)?

Show outdated Hide outdated sympy/codegen/ast.py
Values given by ``limits.h``, x86/IEEE754 defaults if not given.
Default: :attr:`Type.default_limits`.
precision_targets : dict
Maps substitutions for Type.name, e.g. {'integer': 'int64', 'real': 'float32'}

This comment has been minimized.

@asmeurer

asmeurer Jun 7, 2017

Member

Default here too.

@asmeurer

asmeurer Jun 7, 2017

Member

Default here too.

Show outdated Hide outdated sympy/codegen/ast.py
Absolute tolerance. (will be deduced if not given).
limits : dict
Values given by ``limits.h``, x86/IEEE754 defaults if not given.
Default: :attr:`Type.default_limits`.

This comment has been minimized.

@asmeurer

asmeurer Jun 7, 2017

Member

I guess this isn't in Sphinx yet, but will this give the actual value of the attribute in the docs? That would be ideal? Otherwise, you should show what the dict should look like here.

@asmeurer

asmeurer Jun 7, 2017

Member

I guess this isn't in Sphinx yet, but will this give the actual value of the attribute in the docs? That would be ideal? Otherwise, you should show what the dict should look like here.

Show outdated Hide outdated sympy/codegen/ast.py
symbol : Symbol
If a ``Variable`` instance is given as symbol, said instance is simply returned.
type_ : Type (optional)
Type of the variable. Inferred from ``symbol`` if not given.

This comment has been minimized.

@asmeurer

asmeurer Jun 7, 2017

Member

I don't know how I feel about the assumptions being used here.

Also, what about no type, for languages that don't have types, or have type inference?

@asmeurer

asmeurer Jun 7, 2017

Member

I don't know how I feel about the assumptions being used here.

Also, what about no type, for languages that don't have types, or have type inference?

Show outdated Hide outdated sympy/codegen/ast.py
return self.args[2]
class Pointer(Basic):

This comment has been minimized.

@asmeurer

asmeurer Jun 7, 2017

Member

Should it be a subtype of Variable?

@asmeurer

asmeurer Jun 7, 2017

Member

Should it be a subtype of Variable?

Show outdated Hide outdated sympy/printing/ccode.py
def _print_ceiling(self, expr):
return self._print_math_func(expr)
def _print_floor(self, expr):

This comment has been minimized.

@asmeurer

asmeurer Jun 7, 2017

Member

You can just assign these all: _print_floor = _print_ceiling = ... = _print_math_func.

@asmeurer

asmeurer Jun 7, 2017

Member

You can just assign these all: _print_floor = _print_ceiling = ... = _print_math_func.

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Jun 7, 2017

Member

Seems weird to have lots of flags in .args as booleans. Not sure how I feel about that.

Member

asmeurer commented Jun 7, 2017

Seems weird to have lots of flags in .args as booleans. Not sure how I feel about that.

@bjodah bjodah referenced this pull request Jun 7, 2017

Merged

GLSL code printer #12713

Show outdated Hide outdated sympy/printing/tests/test_fcode.py
check(Type('real'), "real(4)", precision=float32)
check(Type('real'), "real(4)", precision=6)
check(Type('real'), "real(8)", precision=float64)
check(Type('real'), "real(8)", precision=15)

This comment has been minimized.

@isuruf

isuruf Jun 27, 2017

Member

In SymPy, we use precision for binary precision and dps for decimal precision. It'd be good to keep that for consistency. See Float.__new__ for an example.

@isuruf

isuruf Jun 27, 2017

Member

In SymPy, we use precision for binary precision and dps for decimal precision. It'd be good to keep that for consistency. See Float.__new__ for an example.

This comment has been minimized.

@bjodah

bjodah Jun 27, 2017

Member

Thanks, agreed. I inherited the terminology from the old printers (but I never liked that printer option anyway) -- I will change to dps.

@bjodah

bjodah Jun 27, 2017

Member

Thanks, agreed. I inherited the terminology from the old printers (but I never liked that printer option anyway) -- I will change to dps.

This comment has been minimized.

@bjodah

bjodah Jun 27, 2017

Member

OK, this is going to be trickier than anticipated. Since precision is already an option in the printers this may need a deprecation cycle? Not sure how to proceed.

@bjodah

bjodah Jun 27, 2017

Member

OK, this is going to be trickier than anticipated. Since precision is already an option in the printers this may need a deprecation cycle? Not sure how to proceed.

This comment has been minimized.

@bjodah

bjodah Jun 27, 2017

Member

We could start raising SymPyDeprecationWarning when a number is passed as precision?

@bjodah

bjodah Jun 27, 2017

Member

We could start raising SymPyDeprecationWarning when a number is passed as precision?

This comment has been minimized.

@asmeurer

asmeurer Jun 27, 2017

Member

Actually it's usually just prec for binary precision. We only used the longer name precision in Float because we have to change the behavior (previously prec was decimal precision). (side note, I see no deprecation issue was opened for this)

@asmeurer

asmeurer Jun 27, 2017

Member

Actually it's usually just prec for binary precision. We only used the longer name precision in Float because we have to change the behavior (previously prec was decimal precision). (side note, I see no deprecation issue was opened for this)

This comment has been minimized.

@bjodah

bjodah Jun 27, 2017

Member

So should we introduce a new printer setting kwarg prec (taking a Type instance) and deprecate the use of printer setting named precision? (the Type will carry the dps info)

@bjodah

bjodah Jun 27, 2017

Member

So should we introduce a new printer setting kwarg prec (taking a Type instance) and deprecate the use of printer setting named precision? (the Type will carry the dps info)

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jul 21, 2017

Member

The upcoming version of glibc (2.26) will support __float128. I think this is a good test for the Type class, do you agree? (I am in the middle of reworking it, will hopefully have something functioning later during the weekend).

Member

bjodah commented Jul 21, 2017

The upcoming version of glibc (2.26) will support __float128. I think this is a good test for the Type class, do you agree? (I am in the middle of reworking it, will hopefully have something functioning later during the weekend).

_default_settings = {
'order': None,
'full_prec': 'auto',
'precision': 15,
'precision': 17,

This comment has been minimized.

@asmeurer

asmeurer Jul 26, 2017

Member

Why did you change this? Assumedly 17 was used so that you always get full precision.

@asmeurer

asmeurer Jul 26, 2017

Member

Why did you change this? Assumedly 17 was used so that you always get full precision.

This comment has been minimized.

@bjodah

bjodah Jul 26, 2017

Member

I changed it from 15 to 17 to preserve precision. There are some issues with printing 17 digits, e.g. the user might find

In [2]: Float(2.1).evalf(17)
Out[2]: 2.1000000000000001

confusing

@bjodah

bjodah Jul 26, 2017

Member

I changed it from 15 to 17 to preserve precision. There are some issues with printing 17 digits, e.g. the user might find

In [2]: Float(2.1).evalf(17)
Out[2]: 2.1000000000000001

confusing

This comment has been minimized.

@asmeurer

asmeurer Jul 26, 2017

Member

But isn't this used to evaluate symbolic constants? Is 15 digits enough to give the full machine precision for any literal?

@asmeurer

asmeurer Jul 26, 2017

Member

But isn't this used to evaluate symbolic constants? Is 15 digits enough to give the full machine precision for any literal?

This comment has been minimized.

@bjodah

bjodah Jul 26, 2017

Member

It used to be 15, now it's 17

@bjodah

bjodah Jul 26, 2017

Member

It used to be 15, now it's 17

This comment has been minimized.

@asmeurer

asmeurer Jul 26, 2017

Member

Oh I somehow read the diff backwards.

@asmeurer

asmeurer Jul 26, 2017

Member

Oh I somehow read the diff backwards.

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Aug 2, 2017

Member

OK, I believe this is ready for review.

Member

bjodah commented Aug 2, 2017

OK, I believe this is ready for review.

Show outdated Hide outdated sympy/codegen/ast.py
@@ -151,7 +193,7 @@ def aug_assign(lhs, op, rhs):
subclass these types are also supported.
op : str
Operator (+, -, /, *, %).
Operator (+, -, /, \*, %).

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Why did you add this?

Also, this sort of change should be failing the tests. I guess my use of python -We:invalid in test_travis.sh isn't working. Probably because the files are bytecompiled already when sympy is installed. I wonder if adding -B as suggested here would fix it.

@asmeurer

asmeurer Aug 2, 2017

Member

Why did you add this?

Also, this sort of change should be failing the tests. I guess my use of python -We:invalid in test_travis.sh isn't working. Probably because the files are bytecompiled already when sympy is installed. I wonder if adding -B as suggested here would fix it.

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Or maybe we just need to use -We:invalid when we call setup.py.

@asmeurer

asmeurer Aug 2, 2017

Member

Or maybe we just need to use -We:invalid when we call setup.py.

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Work here #13083

@asmeurer

asmeurer Aug 2, 2017

Member

Work here #13083

This comment has been minimized.

@bjodah

bjodah Aug 4, 2017

Member

It's a Sphinx thing (asterisk needs to be escaped or it will be a reference), \* is interpreted as \\* by both Python 2 and Python 3. I will change this to \\* though.

@bjodah

bjodah Aug 4, 2017

Member

It's a Sphinx thing (asterisk needs to be escaped or it will be a reference), \* is interpreted as \\* by both Python 2 and Python 3. I will change this to \\* though.

This comment has been minimized.

@asmeurer

asmeurer Aug 4, 2017

Member

In Python 3.6, backslash escapes that don't actually escape anything are a syntax warning, and are tests should fail when they encounter them (it was broken, but I fixed it in #13083). In a future version of Python it will be a syntax error. You can make the docstring a raw string to avoid the issue.

@asmeurer

asmeurer Aug 4, 2017

Member

In Python 3.6, backslash escapes that don't actually escape anything are a syntax warning, and are tests should fail when they encounter them (it was broken, but I fixed it in #13083). In a future version of Python it will be a syntax error. You can make the docstring a raw string to avoid the issue.

self._check(new_val)
delta = new_val - val
if abs(delta) > tol(val): # rounding, e.g. int(3.5) != 3.5

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

I hope you don't run into this issue for high precision (one of these days I'll finish that PR).

@asmeurer

asmeurer Aug 2, 2017

Member

I hope you don't run into this issue for high precision (one of these days I'll finish that PR).

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

Yeah I saw that one, tests have been fine so far though.

@bjodah

bjodah Aug 7, 2017

Member

Yeah I saw that one, tests have been fine so far though.

Show outdated Hide outdated sympy/codegen/ast.py
__slots__ = ['name', 'bits', 'max', 'tiny', 'eps', 'dig', 'decimal_dig']
def _cast_nocheck(self, value):
return Float(str(_sympify(value).evalf(self.decimal_dig)), self.decimal_dig)

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

_sympify disallows strings? Is that what we want?

@asmeurer

asmeurer Aug 2, 2017

Member

_sympify disallows strings? Is that what we want?

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

I was thinking that symbols, e.g. 'x', should be disallowed. But the user could of course pass in e.g. '123.456', changing this to sympify

@bjodah

bjodah Aug 7, 2017

Member

I was thinking that symbols, e.g. 'x', should be disallowed. But the user could of course pass in e.g. '123.456', changing this to sympify

Show outdated Hide outdated sympy/codegen/ast.py
)
float64 = FloatType(
'float64', 64,
max=Float('1.79769313486231571e+308', precision=64+8),

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

This adds three more zeros to the end of the number:

>>> Float('1.79769313486231571e+308', precision=64+8)
1.79769313486231571000e+308

Is that right?

@asmeurer

asmeurer Aug 2, 2017

Member

This adds three more zeros to the end of the number:

>>> Float('1.79769313486231571e+308', precision=64+8)
1.79769313486231571000e+308

Is that right?

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

I've changed the representation of FloatType so max and min etc are now exact.

@bjodah

bjodah Aug 7, 2017

Member

I've changed the representation of FloatType so max and min etc are now exact.

This comment has been minimized.

@asmeurer

asmeurer Aug 8, 2017

Member

Nice I like that a lot better.

@asmeurer

asmeurer Aug 8, 2017

Member

Nice I like that a lot better.

Show outdated Hide outdated sympy/codegen/ast.py
nargs = (2, 3) # type is optional
def __new__(cls, symbol, attrs=None, type_=None):

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Why not just set attrs=FiniteSet() here (or S.EmptySet)?

@asmeurer

asmeurer Aug 2, 2017

Member

Why not just set attrs=FiniteSet() here (or S.EmptySet)?

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

Agreed, updated

@bjodah

bjodah Aug 7, 2017

Member

Agreed, updated

Show outdated Hide outdated sympy/codegen/ast.py
args = (_sympify(symbol), FiniteSet() if attrs is None else FiniteSet(*attrs))
if type_ is not None:
if not isinstance(type_, Type):
raise NotImplementedError("Expected a Type as type_")

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

This is a confusing error message. How about "TypeError("type_ argument should be an instance of Type")` (which is maybe also confusing...)

@asmeurer

asmeurer Aug 2, 2017

Member

This is a confusing error message. How about "TypeError("type_ argument should be an instance of Type")` (which is maybe also confusing...)

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

Agreed, updated

@bjodah

bjodah Aug 7, 2017

Member

Agreed, updated

Show outdated Hide outdated sympy/codegen/ast.py
def __new__(cls, var, val=None, cast=False):
if not isinstance(var, Variable):
raise NotImplementedError("Expected a Variable instance as var")

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Same here (and should be TypeError, assumedly)

@asmeurer

asmeurer Aug 2, 2017

Member

Same here (and should be TypeError, assumedly)

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

Agreed, updated

@bjodah

bjodah Aug 7, 2017

Member

Agreed, updated

Show outdated Hide outdated sympy/codegen/ast.py
if len(args) == 1 and not kwargs and isinstance(args[0], cls):
return args[0]
args = args + tuple([kwargs[k] for k in cls.__slots__[len(args):]])
return Type.__xnew_cached_(cls, *args)

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Token

@asmeurer

asmeurer Aug 2, 2017

Member

Token

Show outdated Hide outdated sympy/codegen/ast.py
setattr(obj, attr, arg)
return obj
__xnew_cached_ = staticmethod(cacheit(__new_stage2__))

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

I think we don't need all this stage2 junk here (or even caching, for that matter). As far as I can tell, it's used in Symbol so that symbols are cached based on derived assumptions (as opposed to the assumptions passed in by the user).

@asmeurer

asmeurer Aug 2, 2017

Member

I think we don't need all this stage2 junk here (or even caching, for that matter). As far as I can tell, it's used in Symbol so that symbols are cached based on derived assumptions (as opposed to the assumptions passed in by the user).

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

Sure, updated

@bjodah

bjodah Aug 7, 2017

Member

Sure, updated

Show outdated Hide outdated sympy/codegen/ast.py
return True
def __hash__(self):
return hash(tuple([getattr(self, attr) for attr in self.__slots__]))

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Should define _hashable_content instead of __hash__.

@asmeurer

asmeurer Aug 2, 2017

Member

Should define _hashable_content instead of __hash__.

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

Looking into Basic it seems to me I need both. I set _hashable_content = hash now.

@bjodah

bjodah Aug 7, 2017

Member

Looking into Basic it seems to me I need both. I set _hashable_content = hash now.

This comment has been minimized.

@asmeurer

asmeurer Aug 8, 2017

Member

You should just set

def _hashable_content(self):
     return tuple([getattr(self, attr) for attr in self.__slots__])

Does that not work?

@asmeurer

asmeurer Aug 8, 2017

Member

You should just set

def _hashable_content(self):
     return tuple([getattr(self, attr) for attr in self.__slots__])

Does that not work?

This comment has been minimized.

@bjodah

bjodah Aug 8, 2017

Member

this is weird, using ./bin/test it works, but when I used pytest I got:

...
  File "/home/bjorn/vc/sympy/sympy/printing/ccode.py", line 168, in C89CodePrinter
    integer: intc
TypeError: unhashable type: 'Type'
@bjodah

bjodah Aug 8, 2017

Member

this is weird, using ./bin/test it works, but when I used pytest I got:

...
  File "/home/bjorn/vc/sympy/sympy/printing/ccode.py", line 168, in C89CodePrinter
    integer: intc
TypeError: unhashable type: 'Type'

This comment has been minimized.

@bjodah

bjodah Aug 8, 2017

Member

Correction: it seems to be a Python2 vs. Python3 thing rather

@bjodah

bjodah Aug 8, 2017

Member

Correction: it seems to be a Python2 vs. Python3 thing rather

This comment has been minimized.

@bjodah

bjodah Aug 8, 2017

Member

So in Python 3 one needs to set __hash__ explicitly if one define __eq__.

@bjodah

bjodah Aug 8, 2017

Member

So in Python 3 one needs to set __hash__ explicitly if one define __eq__.

This comment has been minimized.

@asmeurer

asmeurer Aug 8, 2017

Member

Oh right. I think you have to define __hash__ and have it call super(Type, self).__hash__. There are other examples in the codebase.

@asmeurer

asmeurer Aug 8, 2017

Member

Oh right. I think you have to define __hash__ and have it call super(Type, self).__hash__. There are other examples in the codebase.

Show outdated Hide outdated sympy/codegen/ast.py
))
__str__ = __repr__
_sympystr = lambda self, printer: repr(self)

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Is this not the default anyway?

@asmeurer

asmeurer Aug 2, 2017

Member

Is this not the default anyway?

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

I would do the reverse: put the printing logic in here (so that it can recursively use the printer), and define __repr__ based on _sympystr.

@asmeurer

asmeurer Aug 2, 2017

Member

I would do the reverse: put the printing logic in here (so that it can recursively use the printer), and define __repr__ based on _sympystr.

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

Changed it, repr() does not give a string that can be eval:ed any more though.

@bjodah

bjodah Aug 7, 2017

Member

Changed it, repr() does not give a string that can be eval:ed any more though.

Show outdated Hide outdated sympy/codegen/ast.py
class FloatType(Type):
""" Represents a floating point value.

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Document what all the attributes mean.

@asmeurer

asmeurer Aug 2, 2017

Member

Document what all the attributes mean.

Show outdated Hide outdated sympy/codegen/ast.py
ValueError: Maximum value for data type smaller than new value.
"""
__slots__ = ['name', 'bits', 'max', 'tiny', 'eps', 'dig', 'decimal_dig']

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

What is dig? I'm assuming that means "digits", but how is it different from decimal_dig? And if that does mean that, I think dps is a more standard abbreviation used in SymPy for decimal digits.

@asmeurer

asmeurer Aug 2, 2017

Member

What is dig? I'm assuming that means "digits", but how is it different from decimal_dig? And if that does mean that, I think dps is a more standard abbreviation used in SymPy for decimal digits.

delta = new_val - val
if abs(delta) > tol(val): # rounding, e.g. int(3.5) != 3.5
raise ValueError("Casting gives a significantly different value.")

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Do you have a test that this doesn't fail if the user provides more digits than necessary?

@asmeurer

asmeurer Aug 2, 2017

Member

Do you have a test that this doesn't fail if the user provides more digits than necessary?

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

Added.

@bjodah

bjodah Aug 7, 2017

Member

Added.

Show outdated Hide outdated sympy/codegen/ast.py
__xnew_cached_ = staticmethod(cacheit(__new_stage2__))
def __eq__(self, other):

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Add tests for the different types to make sure == and != work properly.

@asmeurer

asmeurer Aug 2, 2017

Member

Add tests for the different types to make sure == and != work properly.

@@ -104,6 +123,17 @@ def get_math_macros():
}
def _as_macro_if_defined(meth):

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Add a small docstring here explaining what this does.

@asmeurer

asmeurer Aug 2, 2017

Member

Add a small docstring here explaining what this does.

Show outdated Hide outdated sympy/printing/codeprinter.py
else:
raise ValueError("The code-printer setting 'enable_statements' is False,\n"
"but %s generates a statement in the chosen language." %
type(expr))

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

Maybe this is too pedantic, but expressions themselves are valid statements in many languages (such as Python).

@asmeurer

asmeurer Aug 2, 2017

Member

Maybe this is too pedantic, but expressions themselves are valid statements in many languages (such as Python).

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

That's true, but it's a decorator that will be used in respective printer (would we ever want to print an expression as a statement? -- only use case I can think of is in try/except blocks and then enable_statements will be True any way. I guess we could have an option called "only_expressions" or something instead? I find "enable_statements" to be clearer though (minus the fact that an expression could also be a statement).

@bjodah

bjodah Aug 7, 2017

Member

That's true, but it's a decorator that will be used in respective printer (would we ever want to print an expression as a statement? -- only use case I can think of is in try/except blocks and then enable_statements will be True any way. I guess we could have an option called "only_expressions" or something instead? I find "enable_statements" to be clearer though (minus the fact that an expression could also be a statement).

This comment has been minimized.

@asmeurer

asmeurer Aug 8, 2017

Member

Maybe both are misleading because neither expressions or statements are a subset of the other in general. Even in C, for instance, you can't really say an expression is a statement because you need a semicolon to make a statement. And that matters because if you have C_CODE_TEMPLATE.format(expr=ccode(expr)) then it matters if expr should be printed as an expression or statement simply by the question of whether ccode should print a semicolon or not.

@asmeurer

asmeurer Aug 8, 2017

Member

Maybe both are misleading because neither expressions or statements are a subset of the other in general. Even in C, for instance, you can't really say an expression is a statement because you need a semicolon to make a statement. And that matters because if you have C_CODE_TEMPLATE.format(expr=ccode(expr)) then it matters if expr should be printed as an expression or statement simply by the question of whether ccode should print a semicolon or not.

This comment has been minimized.

@bjodah

bjodah Aug 8, 2017

Member

When in doubt I think the printer should just assume that it is dealing with an expression (e.g ccode(x**2) shouldn't append a semicolon). An alternative would be to enable printing of statements in a subclass of the CodePrinter, but due to backward compatibility CodePrinter will need to continue to support printing statements.

@bjodah

bjodah Aug 8, 2017

Member

When in doubt I think the printer should just assume that it is dealing with an expression (e.g ccode(x**2) shouldn't append a semicolon). An alternative would be to enable printing of statements in a subclass of the CodePrinter, but due to backward compatibility CodePrinter will need to continue to support printing statements.

Show outdated Hide outdated sympy/printing/ccode.py
known = self.known_functions[expr.__class__.__name__]
if not isinstance(known, str):
for cb, name in known:
if cb(*expr.args):

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

What does this do? Is there an example of this?

@asmeurer

asmeurer Aug 2, 2017

Member

What does this do? Is there an example of this?

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

values in the known_functions dict may be functions. (e.g. "Abs")

@bjodah

bjodah Aug 7, 2017

Member

values in the known_functions dict may be functions. (e.g. "Abs")

This comment has been minimized.

@asmeurer

asmeurer Aug 8, 2017

Member

Is this new to this PR? Seems to me that anything that's not just a string should be implemented as a printer method.

@asmeurer

asmeurer Aug 8, 2017

Member

Is this new to this PR? Seems to me that anything that's not just a string should be implemented as a printer method.

This comment has been minimized.

This comment has been minimized.

@asmeurer

asmeurer Aug 8, 2017

Member

I see. I'm not a fan of that API (why not just provide a single function?), but I guess we need to keep it.

@asmeurer

asmeurer Aug 8, 2017

Member

I see. I'm not a fan of that API (why not just provide a single function?), but I guess we need to keep it.

Show outdated Hide outdated sympy/printing/ccode.py
if nest:
args = self._print(expr.args[0])
if len(expr.args) > 1:
args += ', %s' % self._print(expr.fromiter(expr.args[1:]))

This comment has been minimized.

@asmeurer

asmeurer Aug 2, 2017

Member

I didn't even know about fromiter. expr.func is more standard.

@asmeurer

asmeurer Aug 2, 2017

Member

I didn't even know about fromiter. expr.func is more standard.

This comment has been minimized.

@bjodah

bjodah Aug 7, 2017

Member

expr.func does not work here. (I was wrong, forgot to unpack)

@bjodah

bjodah Aug 7, 2017

Member

expr.func does not work here. (I was wrong, forgot to unpack)

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Aug 7, 2017

Member

I think this is ready for another round of review.

Member

bjodah commented Aug 7, 2017

I think this is ready for another round of review.

Show outdated Hide outdated sympy/printing/ccode.py
@_as_macro_if_defined
def _print_math_func(self, expr, nest=False):
known = self.known_functions[expr.__class__.__name__]
if not isinstance(known, str):

This comment has been minimized.

@asmeurer

asmeurer Aug 8, 2017

Member

Need to use string_types from sympy.core.compatibility for Python 2 unicode compatibility.

@asmeurer

asmeurer Aug 8, 2017

Member

Need to use string_types from sympy.core.compatibility for Python 2 unicode compatibility.

This comment has been minimized.

@bjodah

bjodah Aug 8, 2017

Member

Got it, thanks.

@bjodah

bjodah Aug 8, 2017

Member

Got it, thanks.

@bjodah bjodah referenced this pull request Aug 9, 2017

Merged

Codegen: More AST nodes #13100

14 of 14 tasks complete

@bjodah bjodah merged commit 16adb22 into sympy:master Aug 10, 2017

1 check passed

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

@bjodah bjodah deleted the bjodah:codegen-ast-type branch Aug 10, 2017

@bjodah bjodah changed the title from [WIP] Add Type, Variable, Pointer & Declaration to .codegen.ast to Add Type, Variable, Pointer & Declaration to .codegen.ast Aug 27, 2017

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