refactor for Codegen Routine #8082

cbm755 · 2014-09-23T11:43:04Z

For OctaveCodeGen (pr #7824), I need some refactoring of Routine.

add Fortran matrix tests
add C matrix tests
move language specific stuff out of Routine.__new__
rework docstrings
rework Result, added NamedResult.
clear-up FIXMEs on name versus result_var in docstrings.

cbm755 · 2014-09-23T13:51:23Z

@jcrist: is this the right approach? I guess you rather mean a standard alone function, not part of the code_gen class.

Lots of tests call Routine() directly: how would all of those know what language? I suspect the main user interface should be "name_expr" into "code_gen" rather than calling Routine...

I'll play with having a standalone routine factory some more later.

jcrist · 2014-09-23T16:35:04Z

I guess you rather mean a standard alone function, not part of the code_gen class.

I think that would be best. Instances of Routine are structural, they define how the generated function should look. The init in Routine itself shouldn't do anything more than ensure that the input is valid. Having a standalone routine function that has a nicer interface (implicit creation of Argument and Result instances as befitting the language of choice, etc...) would allow users to still create routines easily using what Sympy thinks are the best options, while not preventing expert users from initializing Routines directly. Tieing it into the codegen class enforces that the class instance be created by the user just to create a Routine. However, if you were to define one for each CodeGen class, then have a function routine that took in a language, initialized the class inside the function, then called codegen.routine, that would be fine. Actually, I think that's probably the best method - keeps it easily extensible to new languages.

jcrist · 2014-09-23T16:35:39Z

Thanks for taking care of this, the codegen stuff is a bit messy and needs more cleanup than I did in my first pass.

cbm755 · 2014-09-24T12:29:04Z

@jcrist How about this? In the other pr, OctaveCodeGen has its own .routine

It looks like a big change, but its just a copy-paste of the existing Routine.__init__ code.

cbm755 · 2014-09-24T15:44:25Z

Ok, please review. I can rebase commits a bit before merging (if wanted).

jcrist · 2014-09-30T03:37:59Z

sympy/utilities/autowrap.py

    CodeGenArgumentListError, Result, ResultBase, CCodeGen
 )
+from sympy.utilities.codegen import routine as Routine


I think we should be explicit with this change. Disguising the function as a class doesn't seem like a good idea to me.

On 30/09/14 03:38, Jim Crist wrote:

I think we should be explicit with this change.

Agree, done.

cbm755 · 2014-09-30T20:00:26Z

Perhaps rout or something else? the_routine doesn't sound quite right.

rout looks rather like "R Out". Is this a good enough reason to call the friendly function something else? assemble_routine(), make_routine()? This might be nicer:

routines = []
routine = assemble_routine(name, expr, ...)
routines.append(routine)
...
for routine in routines:
     ...

cbm755 · 2014-09-30T20:08:40Z

Why can't these all be in the same import? Not super important though.

Should be fixed.

cbm755 · 2014-09-30T21:58:07Z

I've looked at Result a bit more and it seems there are indeed language-specific assumptions made there. For example, it is not possible to give one a name, rather it automatically generates a hash name. This is fine for C/Fortran but not good for Octave which doesn't have return (blah) but rather has named return values.

Currently I'm (ab)using OutputArguments for Octave return values. I assume the "proper" thing is to fix Result? firstly by subclassing from Variable.

I assume OutputArguments should be only used for "rhs-like" stuff, stuff that should be an argument but for pointers.

cbm755 · 2014-09-30T22:02:50Z

I've added input checking for Routine init in 4dcaa33. @jcrist does that look about right?

jcrist · 2014-09-30T22:56:37Z

rout looks rather like "R Out". Is this a good enough reason to call the friendly function something else? assemble_routine(), make_routine()?

Two options then, leaving it up to you. Either name the routine function something else (I like make_routine best), or just import it as make_routine to the autowrap file, and leave the name the same. Although I suppose that wouldn't work in codegen. However, having a variable in a function called routine is fine if the function doesn't call the routine function. Scoped variables and all.

I assume OutputArguments should be only used for "rhs-like" stuff, stuff that should be an argument but for pointers.

In my mind the distinction is this: Result should be things that are returned. func(x, y) -> (x + y) returns x + y. This means that it can be used in a larger expression as a representation of this calculation, and still have everything work. OutputArguments can't, because they are modified inplace. So using them for everything in Octave may work, but it's not the best.

I assume the "proper" thing is to fix Result? firstly by subclassing from Variable.

That could maybe work. Ideally Result could have an optional name, and an expression that is being returned. That way it could work with C or Octave. Or you could just give it a junk name. Something like "return".

cbm755 · 2014-10-01T11:24:23Z

@jcrist commit 1014510 has my attempt at Result changes. Codegen will need a way to tell the difference between auto-named and user-named, so I subclassed a NamedResult. This works well for Octave.

C/Fortran are not effected. If you give them a NamedResult, they should treat it same as Result, although I didn't add a test for that (should I?)

cbm755 · 2014-10-01T16:45:16Z

Rebased, hopefully easier to understand. No overall changes.

jcrist · 2014-10-06T13:14:38Z

sympy/utilities/codegen.py

+        # Check that all symbols in the expressions are covered by
+        # InputArguments/InOutArguments---subset because user could
+        # specify additional (unused) InputArguments or local_vars.
+        assert symbols.issubset(input_symbols.union(local_vars))


We could do better than an assert for this, and throw an actual error message that will be more meaningful to the users.

We could do better than an assert for this

done.

cbm755 · 2014-10-10T23:21:39Z

Ok, I have removed NamedResult.

After a previous discussion with @moorepants, I'm unsure whether I should merge or rebase for the merge conflict. @jcrist: Preference? if not, I will rebase.

jcrist · 2014-10-14T03:11:06Z

Looks good to me. It would be nice to squash some of these commits together (specifically to remove the unused NamedResult history). Squashing will lose some of the line comments on github, which is why it should be avoided if possible. In this case I think it should be done though. For future cases, simply merging to fix the conflict is preferred.

cbm755 · 2014-10-14T22:11:03Z

rebase done, no changes, just some squashing.

jcrist · 2014-10-14T22:13:33Z

Will merge in 24 hours if no dissents.

moorepants · 2014-10-14T22:45:13Z

sympy/utilities/codegen.py

-           precision  --  FIXME
+        """Return a new variable.
+
+        name : Symbol or MatrixSymbol


Is this missing:

Parameters ==========

?

jcrist · 2014-10-14T23:01:02Z

@moorepants (since you're looking at it right now), and others: I'm +1 on this, but have one nagging thought. The old Routine class did a lot of magic beforehand that will be taken out and moved to a separate function in this PR. This is good. However, anyone that was relying on this magic being in the class Routine will have broken code after this PR.

In general I think this is unavoidable. Further, I don't think anyone uses this functionality anyway. codegen, autowrap, and ufuncify (the major user interfaces to this submodule) won't change, but "expert" users will have a different interface than they did before this PR. I don't think we need (and am actually against) a deprecation warning on this. Something in the release notes would be good though.

moorepants · 2014-10-14T23:05:14Z

I fine with a nice big release note warning. The advanced users would be likely to be using the master branch and would notice this. I have no good idea about how much use of Routine is in the wild.

@bjodah What do you think of this? No deprecation cycle?

bjodah · 2014-10-15T12:19:23Z

👍 I'm fine with just a warning. It's nice to see the codegen stuff moving forward - good job!

jcrist · 2014-10-15T19:16:28Z

There are merge conflicts (I assume from the PR that Jason just merged). Please fix them (could also take this time to squash your last 3 fix-it commits together, but only if you feel like it). I'll merge once that's done.

Put the existing Fortran/C `Routine.__init__` into Codegen class. Essentially unchanged. Codegen will now make appropriate instances of Routine. Routine is still essentially language agnostic, although a particular instance might be more appropriate for a particular Codegen.

Lines to 75 chars, follow numpy style. Some pylint fixes. Slightly simplify Argument and its subclasses.

1. Result subclasses Variable so it has a name. By default, name and result_var get the dummy "result_" symbol. This was already the case for result_var. 2. Add name and result_var kwargs: name defaults to None and result_var defaults to name. 3. Result can work with Matrix class. Result can take dimensions kwarg. Maybe not allowed in C/Fortran but others languages might allow this. 4. Document the difference between name and result_var. Perhaps these could/should be refactored, but for now, document that they are usually the same thing.

cbm755 · 2014-10-15T20:48:52Z

Ok, rebased. Please wait for tests to complete (the other merge meant more Routine->make_routine changes, want to make sure I caught them all).

jcrist · 2014-10-16T03:19:08Z

Merging. @cbm755, please add something in the release notes about this change. Thanks for your work!

refactor for Codegen Routine

cbm755 · 2014-10-16T21:46:01Z

please add something in the release notes about this change.

Ok, done, although might be a bit verbose for what is likely a minor issue! Feel free to edit.

cbm755 mentioned this pull request Sep 23, 2014

Octave/Matlab codegen and code printing #7824

Merged

24 tasks

cbm755 force-pushed the routine_refactor branch from 9e50f03 to 377e2b2 Compare September 24, 2014 12:27

cbm755 mentioned this pull request Sep 24, 2014

Codegen: Matrixslice support #8094

Merged

cbm755 changed the title ~~[WIP] refactor for Codegen Routine~~ refactor for Codegen Routine Sep 29, 2014

jcrist reviewed Sep 30, 2014
View reviewed changes

cbm755 mentioned this pull request Sep 30, 2014

Codegen: Use prefix as the function name in simple cases #8148

Merged

cbm755 force-pushed the routine_refactor branch from 0dbc73d to 16eac46 Compare September 30, 2014 22:28

cbm755 force-pushed the routine_refactor branch from b23d54b to 6815ca8 Compare October 1, 2014 11:04

cbm755 force-pushed the routine_refactor branch from da04700 to 19a38f6 Compare October 1, 2014 16:42

jcrist reviewed Oct 6, 2014
View reviewed changes

cbm755 force-pushed the routine_refactor branch from d82f8b2 to 9cac813 Compare October 14, 2014 22:05

moorepants reviewed Oct 14, 2014
View reviewed changes

cbm755 added 4 commits October 15, 2014 21:27

FCodegen: add matrix tests

2dffcaa

FCodeGen: add MatrixSlice tests, including XFAIL one

d7afc14

CCodegen: add Matrix tests

0379138

cbm755 force-pushed the routine_refactor branch from 1d0a34e to bba1454 Compare October 15, 2014 20:42

cbm755 added 4 commits October 15, 2014 21:45

Codegen: update tests to use "make_routine" instead of Routine directly

711272b

Codegen: redo docstrings, slightly simplify Argument classes

dd09a0c

Lines to 75 chars, follow numpy style. Some pylint fixes. Slightly simplify Argument and its subclasses.

Codegen: do input checking in the Routine class

eed51c4

cbm755 force-pushed the routine_refactor branch from bba1454 to eb38f4e Compare October 15, 2014 20:46

jcrist added a commit that referenced this pull request Oct 16, 2014

Merge pull request #8082 from cbm755/routine_refactor

1868312

refactor for Codegen Routine

jcrist merged commit 1868312 into sympy:master Oct 16, 2014

cbm755 deleted the routine_refactor branch October 16, 2014 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor for Codegen Routine #8082

refactor for Codegen Routine #8082

cbm755 commented Sep 23, 2014

cbm755 commented Sep 23, 2014

jcrist commented Sep 23, 2014

jcrist commented Sep 23, 2014

cbm755 commented Sep 24, 2014

cbm755 commented Sep 24, 2014

jcrist Sep 30, 2014

cbm755 Sep 30, 2014

cbm755 commented Sep 30, 2014

cbm755 commented Sep 30, 2014

cbm755 commented Sep 30, 2014

cbm755 commented Sep 30, 2014

jcrist commented Sep 30, 2014

cbm755 commented Oct 1, 2014

cbm755 commented Oct 1, 2014

jcrist Oct 6, 2014

cbm755 Oct 9, 2014

cbm755 commented Oct 10, 2014

jcrist commented Oct 14, 2014

cbm755 commented Oct 14, 2014

jcrist commented Oct 14, 2014

moorepants Oct 14, 2014

jcrist commented Oct 14, 2014

moorepants commented Oct 14, 2014

bjodah commented Oct 15, 2014

jcrist commented Oct 15, 2014

cbm755 commented Oct 15, 2014

jcrist commented Oct 16, 2014

cbm755 commented Oct 16, 2014

refactor for Codegen Routine #8082

refactor for Codegen Routine #8082

Conversation

cbm755 commented Sep 23, 2014

cbm755 commented Sep 23, 2014

jcrist commented Sep 23, 2014

jcrist commented Sep 23, 2014

cbm755 commented Sep 24, 2014

cbm755 commented Sep 24, 2014

jcrist Sep 30, 2014

Choose a reason for hiding this comment

cbm755 Sep 30, 2014

Choose a reason for hiding this comment

cbm755 commented Sep 30, 2014

cbm755 commented Sep 30, 2014

cbm755 commented Sep 30, 2014

cbm755 commented Sep 30, 2014

jcrist commented Sep 30, 2014

cbm755 commented Oct 1, 2014

cbm755 commented Oct 1, 2014

jcrist Oct 6, 2014

Choose a reason for hiding this comment

cbm755 Oct 9, 2014

Choose a reason for hiding this comment

cbm755 commented Oct 10, 2014

jcrist commented Oct 14, 2014

cbm755 commented Oct 14, 2014

jcrist commented Oct 14, 2014

moorepants Oct 14, 2014

Choose a reason for hiding this comment

jcrist commented Oct 14, 2014

moorepants commented Oct 14, 2014

bjodah commented Oct 15, 2014

jcrist commented Oct 15, 2014

cbm755 commented Oct 15, 2014

jcrist commented Oct 16, 2014

cbm755 commented Oct 16, 2014