Code Printing refactor, matrix support #7823

jcrist · 2014-08-04T21:42:14Z

** Edit **
This started out as just adding support for Matrix in codeprinting/codegen, but turned into a fairly substantial refactor of the existing codebase. I have updated this to reflect that. The TODO list is a trimmed down version of the original TODO, autowrap support should come in a later PR, this only has to do with printing now.

See my mammoth sized comment below for what's inside this PR now
** end edit **

TODO:

Support for matrix output in ccode and CCodeGen
Support for matrix output in fcode and FCodeGen
Support for MatrixSymbol input in ccode and CCodeGen
Support for MatrixSymbol input in fcode and FCodeGen
Tests

Works for C code only, with input and input/output args of matrices. Expression to be codegen'ed must be an Eq(MatrixSymbol, Matrix). Still need to work through codegen code to see if there is an easier (less hackish) way, but it currently "works".

Now supports automatic output variable creation for Matrices not originally wrapped in an `Eq`. This means that the following now works: mat = Matrix([a, b, c]) codegen(('mat', mat), 'c', 'mat) Also did some code cleanup to prior work on matrices.

moorepants · 2014-08-04T22:02:38Z

On WIPs it may be useful to check this out: http://docs.travis-ci.com/user/how-to-skip-a-build/

You can prevent Travis from building when you don't need it to.

jcrist · 2014-08-04T22:05:09Z

Ah, good to know. I can't cancel them directly as I don't have push access. I'll try that next time.

moorepants · 2014-08-04T22:11:14Z

You can also ask @asmeurer for push access. You should get it by now.

Start of refactor to codeprinters to make adding new features simpler. The main purpose behind this is to make support for matrices and matrix expressions easier, and remove bugs in Piecewise and looping. - Added Assignment node, to represent assignment rather than Equality (which should be used for equality testing, resulting in a bool). - Refactored ccode `doprint` method to move most of the control flow to a _print_Assignment method, called like all other print methods by the _print method in Printer. This makes more sense, and sticks with the original design of the Printer class. - Some variable name cleanups to improve readability of the CodePrinter class.

Refactored fcode similar to how ccode was changed in the last commit. Also changed doprint to accept assign_to as a kwarg, as ccode does, rather than store it as a setting.

Fortran code printing now supports 2 dimensional arrays. Also added (possible) efficiency improvement by reordering declaration order of C code array elements to be in-memory order. gcc probably optimizes this already, but it was an easy fix.

- Moved common methods onto CodePrinter class - We now check if either the lhs or rhs of an expression has IndexedBase objects to see if loops are needed. This is necessary because sometimes the accumulator isn't an Indexed Object. There were two thoughts on how to set this up: 1. Call _doprint_a_piece (now _doprint_loops) on *all* Assignment expressions: - Expressions w/o loops: 2 tree traversals before being printed - Expressions w/ loops: 2 tree traversals before being printed 2. Check for IndexedBase in lhs first, then rhs: - Expressions w/o loops: 1 tree traversal before being printed - Expressions w/ loops: 3 tree traversals before being printed (worst case. Most cases will be only 2). Note that most Indexed expressions tend to be short and concises, as well as have a lhs which contains Indexed objects (will result in *fast* method call, because lhs trees are small). In contrast, most users do not use Indexed, and many expressions are rather large. Hence it is more beneficial for the user to be cheaper on loopless expressions, while increasing the worst-case time for looped expressions (most looped expressions will have a negligible time increase).

Continuation of the codegen refactor. jscode now in same format as ccode and fcode. jscode now also supports matrices.

- Moved all common methods into CodePrinter - Added methods that need to be defined by anything subclassed from `CodePrinter` to `CodePrinter` with a `NotImplementedError`. This should help anyone out looking to create a new code printer, as it shows what needs to be defined by the subclass so that everything just *works* - Small fix to the mathematic printing so it has it's own doprint method. I don't know anything about mathematica. This keeps everything the same as before for this refactoring. - Small formatting cleanups

Now Piecewise lacking a default condition (i.e. an (expr, True) term) will throw an error. This is because without such a statement, the generated code may not return anything. Other options were to return NaN in such a situation, but this was turned down due to lack of support by all languages/compilers, and that this is unexpected behavior. Added tests for the conditional printing.

Rewrote `Assignment` operator to subclass Relational, not Equality. This reduces object creation time, allowed more types to be used as the rhs, and makes more sense in the end. Added tests for Assignment. This class should probably go in a different file, but is in codeprinter for now. Added tests for matrix code printing to ccode, fcode and jscode.

jcrist · 2014-08-10T05:14:02Z

I set out just to get matrix printing working, but ended up finding so many bugs in the existing codebase that I ended up doing a substantial refactor. This PR now only has to do with codeprinting (and a little fix to the codegen stuff), but nothing with autowrap. Instead I rewrote a good chunk of the codeprinting code.

What's in this PR:

Matrix Printing support for ccode, fcode, and jscode
Support for MatrixSymbol and MatrixElement inside expressions
Refactor of codeprinter to use the visitor pattern, rather than lots of special cased stuff
Fixes to Piecewise printing so that it prints valid code in all cases, or errors if it can't (fixes Codegen fails with Piecewise functions in C #7791)
A new Assignment class, currently only used internally inside the codeprinter. This lives in codeprinter.py for now, but perhaps should be moved.
Tests for all new functionality

Changes in codeprinting philosophy

In the past, we'd print expressions haphazardly, doing assignment on some, and not on others. It seemed messy and inconsistent to me, so I rewrote it. The new philosophy is as follows:

If an assign_to kwarg is given, return code that assigns expr to assign_to
Otherwise, return code that can be prepended with "assign_to =", and still have a valid statement.

This means that we will no longer be printing code that looks like this:

if (a > 0) {
    1
}
else {
    2
}

as this in practice doesn't do anything. It may be a translation of Piecewise((1, a>0), (2, True)), but it's meaningless. Prepending an "assign_to = " will not result in valid code. In this case, in the lack of an assign_to kwarg the printer will try to use inline conditionals (ternary operator in C/javascript, merge in Fortran) Check out the Piecewise tests for ccode, jscode, and fcode to see how the new form will look.

Changes in codeprinting implementation

The new implementation moves almost all complicated code to the codeprinter class. Printers then subclass this, as they did before, but only implement printing for types (as well as some needed functions). The doprint function is stripped of all printing operations and now uses the visitor pattern to print the expressions (as done by other printers). The Assignment type is used to carry the assign_to kwarg through the printer so each type can have special printing if it's being used in an assignment. This results in simpler code, and also increased functionality (nested Piecewise operations, or Piecewise in Matrices/Matrices in Piecewise). None of these changes are user facing, so no worry here.

User facing changes

Besides the added functionality, the only user facing changes are as follows:

Introduction of standard kwarg in fcode printer. This is simply an integer for the standard as used colloquially (95, 77, etc...). Currently, the only distinction is made between 95 and newer, and prior to 95. This is used to determine whether the merge function is available for inline conditionals. Default is 77.
Conditionals will now error if there is not a default expression (i.e. Piecewise ends in (expr, True)). This is to prevent expressions that may not result in anything. For example, prior to this fix a Piecewise of the form Piecewise((1, a > 0), (2, Eq(a, 0))) would be translated into:

int func(int a) {
    int x;
    if (a > 0) {
        x = 1;
    }
    else if (a == 0) {
        x = 2;
    }
    return x;
}

In this form, there would be garbage output if a < 0. By not allowing Piecewise without a default (which translates to an else) we remove this chance.

Moving Forward

I'm sure there will be little things I'll have to fix to get this merged, but this is all the functionality I'd like to have in this PR. In its current state this provides many needed bugfixes to codeprinting, cleans up and simplifies the codebase, and adds support for Matrices and MatrixSymbols in expressions. After this is merged I'll submit another PR adding support for autowrap and the like.

- Fixed formatting to work with python 2.6 - Fixed error in immutable eval_Eq - Added test for Assignment in test_args

jcrist · 2014-08-10T19:02:45Z

@moorepants, @srjoglekar246: please review.

@bjodah: I reworked a good portion of the code printing, but left all the Indexed stuff the same. It should be more extensible in its new form though. Will this work with what you're doing?

srjoglekar246 · 2014-08-10T19:15:12Z

sympy/printing/jscode.py

@@ -239,6 +214,10 @@ def jscode(expr, assign_to=None, **settings):

       expr : sympy.core.Expr
           a sympy expression to be converted
+       assign_to : optional


Maybe write the expected type?

This was just copied from another docstring. Really all the docs need improvement. Will fix.

bjodah · 2014-08-10T20:13:13Z

I really like this PR - I think it's a good improvement to the codeprinters. I do not use the tensor contraction functionality of Indexed so I cannot comment on that but as long as the tests still pass I see no reason to why this wouldn't be a nice improvement even in that case.

moorepants · 2014-08-11T19:45:00Z

sympy/printing/tests/test_ccode.py

+        "else {\n"
+        "   A[1][0] = y;\n"
+        "}\n"
+        "A[2][0] = sin(z);")


Yeah, I'm really happy about this. You can also do Piecewise inline in a matrix element definition using the ternary operators, so you don't even have to apply piecewise_fold. I'm seeing all sorts of uses for this, mainly in the evaluation of a linearized system matrix where the choice of independent/dependent coordinates is the conditional (dealing with the singularities). Would help with stability analysis throughout an entire range of motion.

Totally. This is gonna open lots of possibilities.

moorepants · 2014-08-11T20:07:12Z

This looks good to me. It'd be nice to have a few examples of the new matrix stuff in the sphinx docs to finish this off. You could just copy some over.

@mrocklin Do you have any comments on this?

@tarzzz Please review Jim's PR here.

- Added improved docstrings to fcode, ccode, and jscode - Standardized implementation of all three code printing functions, they now all take the same kwargs (except for standard and source_format, which are fortran specific). - Standardized implementation of Function printing, moved this to codeprinter. There was a bug in that some functions that weren't standard C/Fortran/JS were getting through without being marked. This was because of the old method of "unless it's stated to be bad, it's good". The new printing works the opposite: only functions that are known to be standard are whitelisted. This catches the issues in a much more standard and clean way, and allows for reuse of the print_Function implementation. - As part of standardizing Function printing, the fix for sympy#6814 was changed. Previously we were printing `log(10d0)` for `log(10)` in fortran, because this function only accepts floats. The new implementation evaluates this to the required precision (default 15) in sympy, and just prints a number. If the function doesn't evaluate fully (suppose atan2(2, x) -> atan2(2.0000, x)), then the numeric constants are still printed as doubles, so everything works fine.

jcrist · 2014-08-11T23:50:54Z

I have updated the docstrings with tons of examples (and made them consistent between the three functions). In the process found a few inconsistencies/bugs in ccode, jscode, and fcode, mainly dealing with how user-functions/functions are printed. Because this is already a mammoth of a PR dealing with pretty much an overhaul of the printing I added this in here as well. If you're interested in the specifics, read the latest commit message. Once this passes travis I think it's good to go.

jcrist · 2014-08-12T15:51:39Z

Looks like all tests passed. What are people's thoughts on this? @moorepants, @srjoglekar246.

moorepants · 2014-08-12T16:05:10Z

It looks good to me. Seems like the only major change that people need to be aware of is the conditionals, but that seems to be an improvement to get correct behavior. I guess you could send out a warning if people don't add the default to their Piecewise.

My only comment is that the sphinx docs only include API references which will have your examples, but it would be nice to have some prose to explain things a bit. But not a requirement for merging this.

+1

jcrist · 2014-08-12T16:08:38Z

Right now we error if people don't add the default. Previously if people didn't add the default it would result in uncompilable code (bracket mismatches), so I don't think anyone was using this behavior anyway. All other changes were fixing broken things as well, so I don't think anything needs to be deprecated.

I'll give people the day to look this over, and if no one objects I'll merge it tonight.

moorepants · 2014-08-12T16:10:05Z

It'd be nice to get @mrocklin's opinion.

mrocklin · 2014-08-12T16:28:39Z

Probably won't have the time to look at this until later tonight, around 10 hours from now. Even then it's dubious.

jcrist · 2014-08-13T02:11:55Z

@mrocklin: This is mainly about code generation, and has little to do with other things. The only thing I'm really concerned about is the small change I had to make to the matrix stuff to get Eq to work, as it's used internally in the codeprinting. See the change in immutable.py. Basically this allows Eq to work between an ImmutableMatrix and a MatrixSymbol of the same shape, and remain unevaluated.

mrocklin · 2014-08-13T03:00:48Z

In principle that doesn't cause any blocks or -1s from me. I wouldn't be surprised if it caused trouble down the line. For the time being though it seems like a decent path forward. If issues arise in the future it might make sense to create a matrix equality class.

jcrist · 2014-08-13T03:09:28Z

In reality the need is just a bandaid on a bigger issue that I plan on tackling soon. The only reason Eq is used in the codegen stuff is to represent Assignment, which I think is a bad idea, mainly because it prevents any use of Eq in boolean expression printing down the road. This PR includes the first step in that change by creating a makeshift Assignment class, which is currently only used in the printers. I have some thoughts on matrices and Eq. I'll comment over on the issue in a bit.

asmeurer · 2014-08-13T04:08:30Z

sympy/matrices/immutable.py

@@ -75,6 +75,9 @@ def _eval_Eq(self, other):
        """
        if not hasattr(other, 'shape') or self.shape != other.shape:
            return S.false
+        if isinstance(other, MatrixExpr) and not isinstance(
+                other, ImmutableMatrix):


Are there classes that are instances of both?

ImmutableMatrix is an instance of both.

jcrist · 2014-08-13T21:42:40Z

If there are no objections, I'll merge this tomorrow morning.

tarzzz · 2014-08-14T04:54:38Z

👍

Code Printing refactor, matrix support

certik · 2014-08-17T21:42:19Z

@jcrist thanks for this work! Shouldn't we add tests for more than one condition?

In [7]: print ccode(Piecewise((x, x<1), (x**2, True)))
((x < 1) ? (
   x
)
: (
   pow(x, 2)
))

In [8]: print ccode(Piecewise((x, x<1), (sin(x), x<5), (x**2, True)))
((x < 1) ? (
   x
)
: ((x < 5) ? (
   sin(x)
)
: (
   pow(x, 2)
)))

In [9]: print ccode(Piecewise((x, x<1), (sin(x), x<5), (cos(x), x<9), (x**2, True)))
((x < 1) ? (
   x
)
: ((x < 5) ? (
   sin(x)
)
: ((x < 9) ? (
   cos(x)
)
: (
   pow(x, 2)
))))

To make sure this will continue working. The same for:

In [10]: print ccode(Piecewise((x, x<1), (sin(x), x<5), (cos(x), x<9), (x**2, True)), assign_to="c")
if (x < 1) {
   c = x;
}
else if (x < 5) {
   c = sin(x);
}
else if (x < 9) {
   c = cos(x);
}
else {
   c = pow(x, 2);
}

jcrist added 2 commits July 31, 2014 18:00

Start of work on matrix codegen

c3bf470

Works for C code only, with input and input/output args of matrices. Expression to be codegen'ed must be an Eq(MatrixSymbol, Matrix). Still need to work through codegen code to see if there is an easier (less hackish) way, but it currently "works".

cbm755 mentioned this pull request Aug 5, 2014

Octave/Matlab codegen and code printing #7824

Merged

24 tasks

jcrist added 8 commits August 7, 2014 11:16

Refactored fcode to be more like ccode

e445a6b

Refactored fcode similar to how ccode was changed in the last commit. Also changed doprint to accept assign_to as a kwarg, as ccode does, rather than store it as a setting.

Added support for matrices in Fortran

7bf15ad

Fortran code printing now supports 2 dimensional arrays. Also added (possible) efficiency improvement by reordering declaration order of C code array elements to be in-memory order. gcc probably optimizes this already, but it was an easy fix.

jscode fixes: refactor and matrix support

dea21c0

Continuation of the codegen refactor. jscode now in same format as ccode and fcode. jscode now also supports matrices.

jcrist changed the title ~~[WIP] - Matrix code gen~~ [WIP] - Code Printing refactor, matrix support Aug 10, 2014

Codeprinting: bugfixes

874890e

- Fixed formatting to work with python 2.6 - Fixed error in immutable eval_Eq - Added test for Assignment in test_args

srjoglekar246 reviewed Aug 10, 2014
View reviewed changes

moorepants reviewed Aug 11, 2014
View reviewed changes

moorepants changed the title ~~[WIP] - Code Printing refactor, matrix support~~ Code Printing refactor, matrix support Aug 12, 2014

asmeurer reviewed Aug 13, 2014
View reviewed changes

jcrist added a commit that referenced this pull request Aug 14, 2014

Merge pull request #7823 from jcrist/MatrixCodeGen

f888ebf

Code Printing refactor, matrix support

jcrist merged commit f888ebf into sympy:master Aug 14, 2014

This was referenced Aug 17, 2014

Codegen fails with Piecewise functions in C #7791

Closed

Fix Piecewise() ccode bugs #2524

Closed

sylee957 mentioned this pull request Mar 2, 2019

Mathematica code printer should not use StrPrinter.doprint #16128

Merged

jcrist deleted the MatrixCodeGen branch March 20, 2019 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Printing refactor, matrix support #7823

Code Printing refactor, matrix support #7823

jcrist commented Aug 4, 2014

moorepants commented Aug 4, 2014

jcrist commented Aug 4, 2014

moorepants commented Aug 4, 2014

jcrist commented Aug 10, 2014

jcrist commented Aug 10, 2014

srjoglekar246 Aug 10, 2014

jcrist Aug 10, 2014

bjodah commented Aug 10, 2014

moorepants Aug 11, 2014

jcrist Aug 11, 2014

moorepants Aug 12, 2014

moorepants commented Aug 11, 2014

jcrist commented Aug 11, 2014

jcrist commented Aug 12, 2014

moorepants commented Aug 12, 2014

jcrist commented Aug 12, 2014

moorepants commented Aug 12, 2014

mrocklin commented Aug 12, 2014

jcrist commented Aug 13, 2014

mrocklin commented Aug 13, 2014

jcrist commented Aug 13, 2014

asmeurer Aug 13, 2014

jcrist Aug 13, 2014

jcrist commented Aug 13, 2014

tarzzz commented Aug 14, 2014

certik commented Aug 17, 2014

Code Printing refactor, matrix support #7823

Code Printing refactor, matrix support #7823

Conversation

jcrist commented Aug 4, 2014

moorepants commented Aug 4, 2014

jcrist commented Aug 4, 2014

moorepants commented Aug 4, 2014

jcrist commented Aug 10, 2014

What's in this PR:

Changes in codeprinting philosophy

Changes in codeprinting implementation

User facing changes

Moving Forward

jcrist commented Aug 10, 2014

srjoglekar246 Aug 10, 2014

Choose a reason for hiding this comment

jcrist Aug 10, 2014

Choose a reason for hiding this comment

bjodah commented Aug 10, 2014

moorepants Aug 11, 2014

Choose a reason for hiding this comment

jcrist Aug 11, 2014

Choose a reason for hiding this comment

moorepants Aug 12, 2014

Choose a reason for hiding this comment

moorepants commented Aug 11, 2014

jcrist commented Aug 11, 2014

jcrist commented Aug 12, 2014

moorepants commented Aug 12, 2014

jcrist commented Aug 12, 2014

moorepants commented Aug 12, 2014

mrocklin commented Aug 12, 2014

jcrist commented Aug 13, 2014

mrocklin commented Aug 13, 2014

jcrist commented Aug 13, 2014

asmeurer Aug 13, 2014

Choose a reason for hiding this comment

jcrist Aug 13, 2014

Choose a reason for hiding this comment

jcrist commented Aug 13, 2014

tarzzz commented Aug 14, 2014

certik commented Aug 17, 2014