Adds more flexible code generators for rhs functions that result in faster eval times. #113

Merged
merged 27 commits into from Feb 5, 2015

Conversation

Projects
None yet
3 participants
@moorepants
Member

moorepants commented Jan 29, 2015

This is pretty close to ready. I've put together a better designed code generation system for the guts of the generate_ode_function function. I plan to have this replace everything that exists in pydy/codegen/code. I think I can make it completely backwards compatible too, but I may propose a slight backwards incompatibility.

Backwards incompatibilities:

  • pydy.codegen.code.CythonGenerator is removed. I doubt anyone used this, so I didn't worry about keeping it around. Use pydy.codegen.cython_code.CythonMatrixGenerator instead.

Deprecations:

  • The pydy.codegen.code module will be removed come 0.4.0.
  • The generated rhs functions will not accept `args={'constants': ..., 'specified': ...} come 0.4.0.

Fixes issues: #14, #15, #16

  • Separated the C and Cython code gen and wrapping into two new modules.
  • Generalized the C and Cython code generators to support any number of input arrays and output matrices.
  • Developed an abstract base class ODEFunctionGenerator class that makes use of mixins to create the correct rhs functions for three different types of user input: (1) user provides only the rhs, (2) user provides the full mass matrix and accompanying rhs, and (3) user provides mass matrix and rhs for the udots plus the qdots rhs. It is now relatively easy to create subclasses of this to provide more backends.
  • Created a CythonODEFunctionGenerator subclass.
  • Add support for specified inputs to the ODEFunctionGenerator and subclasses.
  • Create a LambdifyODEFunctionGenerator subclass.
  • Create a TheanoODEFunctionGenerator subclass.
  • Fix bug in SymPy 0.7.4 where Matrix doesn't have free_symbols
  • Fix bug in Cython generator that gets confused loading different modules in a row in the compilation step.
  • The generated RHS function should support a constant map dict or an array of correct length.
  • Add docstrings to the generated rhs functions.
  • Allow the args dict to be supplied to the new rhs functions.
  • Replace guts of generate_ode_function with new code.
  • Check why C code doc string isn't always created.
  • Switch order of inputs/outputs in CMatrixGenerator and CythonMatrixGenerator to match lambdify.
  • Move import errors for optional dependencies into the new respective ODEFunctionGeneratorClasses.
  • Make sure warnings fire for old stuff.
  • Get it working on SymPy 0.7.4.1
  • Support passing in a Generator object to generate_ode_function.
  • Add option to specify which type of inputs you want in rhs so that you can bypass all the slow arg parsing.

Other enhancements I plan to get to in the future:

  • Support creating a function for the Jacobian of the rhs.
  • Support functions of time in the ODEs.
  • Create Generator class for Octave and Matlab. The Octave one can be wrapped with oct2py.
  • Support output equations: both those that share evaluation at each time step and those that can be evaluated after the simulation.
  • Add on disk caching for generated functions with https://github.com/bjodah/pycompilation. This can also replace our compilation code.
  • Make the CSE optional.
  • Support constants=None for EoMs that have numerical values subbed in for all of the constants. You could avoid any of the numerical eval of the constants this way and probably speed things up.

@pydy/pydy-developers I'd love feedback on this, as it is a big change. I'd like to hear what you think of the design and I always fret over function, class, and call signature arg names so thoughts on those are appreciated too.

moorepants added some commits Jan 27, 2015

Adds a new class, CMatrixGenerator, to generalized our C code.
This new class allows one to pass in a list of SymPy Matrix objects and
generate a single function that efficiently numerically evaluates all of the
matrices simultaneously.
Introduces a Cython wrapper generator for the CMatrixGenerator.
- Moved wrap_and_indent into pydy/utils.py
- Moved templates into the CMatrixGenerator class.
- Added new CythonMatrixGenerator class.
Adds ODEFunctionGenerator classes.
- An ODEFunctionGenerator base class along with mixins for the rhs generation
  are added.
- A subclass of ODEFunctionGenerator for Cython based code gen is added.

@moorepants moorepants self-assigned this Jan 29, 2015

@moorepants moorepants added this to the 0.3.0 Release milestone Jan 29, 2015

@moorepants moorepants changed the title from [WIP] Adds more flexible code generators for rhs functions that result in faster eval times. to Adds more flexible code generators for rhs functions that result in faster eval times. Jan 30, 2015

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jan 30, 2015

Member

@chrisdembia @jcrist @gilbertgede @hazelnusse @oliverlee

This is ready to go in. I'd really love some feedback on it. Thanks.

Member

moorepants commented Jan 30, 2015

@chrisdembia @jcrist @gilbertgede @hazelnusse @oliverlee

This is ready to go in. I'd really love some feedback on it. Thanks.

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Jan 30, 2015

Contributor

I was a little confused by what Function in ODEFunctionGenerator meant. It seems that this class provides the RHS of ODE's? ODERHSGenerator, or ODERightHandSide, or CanonicalODE?

Contributor

chrisdembia commented Jan 30, 2015

I was a little confused by what Function in ODEFunctionGenerator meant. It seems that this class provides the RHS of ODE's? ODERHSGenerator, or ODERightHandSide, or CanonicalODE?

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jan 30, 2015

Member

@chrisdembia The purpose of the class is to generate a function that numerically evaluates the right hand side of the ODEs. We had a conversation a while back that resulted in us naming the main function in the code module to generate_ode_function. So Function is in the class and module names because of that.

I'm having trouble finding the earlier naming conversation. This is commit when it was changed: e6025ab

Member

moorepants commented Jan 30, 2015

@chrisdembia The purpose of the class is to generate a function that numerically evaluates the right hand side of the ODEs. We had a conversation a while back that resulted in us naming the main function in the code module to generate_ode_function. So Function is in the class and module names because of that.

I'm having trouble finding the earlier naming conversation. This is commit when it was changed: e6025ab

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jan 30, 2015

Member

Here is the conversation: https://github.com/pydy/pydy-code-gen/issues/8

ODERHSGenerator along with LambdifyODERHSGenerator, etc seems fine to me. It'll shorten things up with no loss in clarity.

Member

moorepants commented Jan 30, 2015

Here is the conversation: https://github.com/pydy/pydy-code-gen/issues/8

ODERHSGenerator along with LambdifyODERHSGenerator, etc seems fine to me. It'll shorten things up with no loss in clarity.

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Jan 31, 2015

Contributor

Ah okay. Sorry for forgetting the history. I think ODEFunctionGenerator is fine though; ODERHS could be confusing.

Contributor

chrisdembia commented Jan 31, 2015

Ah okay. Sorry for forgetting the history. I think ODEFunctionGenerator is fine though; ODERHS could be confusing.

pydy/codegen/ode_function_generator.py
+ self.eval_arrays = eval_arrays
+
+
+def generate_ode_function(right_hand_side, coordinates, speeds, constants,

This comment has been minimized.

@jcrist

jcrist Jan 31, 2015

Member

I assume this is the intended entry point for the user, and all the above classes are just implementation?

@jcrist

jcrist Jan 31, 2015

Member

I assume this is the intended entry point for the user, and all the above classes are just implementation?

This comment has been minimized.

@moorepants

moorepants Jan 31, 2015

Member

The main entry point is this function, but Lambdify/Cython/TheanoODEFunctionGenerator can be too. This function is mainly in place to mirror the function that I removed and follows the SymPy code printers, where a function version is available for the classes.

@moorepants

moorepants Jan 31, 2015

Member

The main entry point is this function, but Lambdify/Cython/TheanoODEFunctionGenerator can be too. This function is mainly in place to mirror the function that I removed and follows the SymPy code printers, where a function version is available for the classes.

pydy/codegen/ode_function_generator.py
+ function's docstring for more details after generation.
+
+ """
+ generators = {'lambdify': LambdifyODEFunctionGenerator,

This comment has been minimized.

@jcrist

jcrist Jan 31, 2015

Member

Perhaps this could be stored in a global, and a register function added? Then a user could create their own generator, and say register("my_generator", MyGenerator), which adds it to the global lookup? This may not be a feature that's ever used/even needed. Just a thought.

@jcrist

jcrist Jan 31, 2015

Member

Perhaps this could be stored in a global, and a register function added? Then a user could create their own generator, and say register("my_generator", MyGenerator), which adds it to the global lookup? This may not be a feature that's ever used/even needed. Just a thought.

This comment has been minimized.

@moorepants

moorepants Jan 31, 2015

Member

That's easy enough. I can add it.

@moorepants

moorepants Jan 31, 2015

Member

That's easy enough. I can add it.

This comment has been minimized.

@jcrist

jcrist Jan 31, 2015

Member

Actually, after more thought, it may be better to just allow generator to be a custom generator object. No globabl registry, seems simpler (perhaps).

@jcrist

jcrist Jan 31, 2015

Member

Actually, after more thought, it may be better to just allow generator to be a custom generator object. No globabl registry, seems simpler (perhaps).

This comment has been minimized.

@moorepants

moorepants Jan 31, 2015

Member

Good idea.

@moorepants

moorepants Jan 31, 2015

Member

Good idea.

pydy/codegen/ode_function_generator.py
+ def _lambdify(self, outputs):
+ # TODO : We could forgo this substitution for speed purposes and
+ # have lots of args for lambdify (like it used to be done) but there
+ # may be some limitations on number of args.

This comment has been minimized.

@jcrist

jcrist Jan 31, 2015

Member

I'm not sure if python incurs a penalty for number of args. I know numpy ufuncs do, but that has to do with their C-api.

@jcrist

jcrist Jan 31, 2015

Member

I'm not sure if python incurs a penalty for number of args. I know numpy ufuncs do, but that has to do with their C-api.

This comment has been minimized.

@moorepants

moorepants Jan 31, 2015

Member

I think someone recently reported this happening to SymPy with a huge number of args in lambdify. There may be a 255 line length limit (maybe only in Python 3?).

@moorepants

moorepants Jan 31, 2015

Member

I think someone recently reported this happening to SymPy with a huge number of args in lambdify. There may be a 255 line length limit (maybe only in Python 3?).

pydy/codegen/ode_function_generator.py
+ def_vecs = ['q', 'u', 'r', 'p']
+
+ for syms, vec_name in zip(self.inputs, def_vecs):
+ v = sm.DeferredVector(vec_name)

This comment has been minimized.

@jcrist

jcrist Jan 31, 2015

Member

This is going away. Please don't use this. If MatrixSymbol can't do it, this should be fixed.

@jcrist

jcrist Jan 31, 2015

Member

This is going away. Please don't use this. If MatrixSymbol can't do it, this should be fixed.

This comment has been minimized.

@moorepants

moorepants Jan 31, 2015

Member

As far as I know MatrixSymbol doesn't work with lambdify yet. Yes it should, but PyDy supports SymPy 0.7.4.1+ right now, so I'm going to keep this in. I wish DeferredVector worked for theano_function too.

@moorepants

moorepants Jan 31, 2015

Member

As far as I know MatrixSymbol doesn't work with lambdify yet. Yes it should, but PyDy supports SymPy 0.7.4.1+ right now, so I'm going to keep this in. I wish DeferredVector worked for theano_function too.

This comment has been minimized.

@jcrist

jcrist Jan 31, 2015

Member

Mmk. I'll try to get lambdify working with MatrixSymbol this week. Then we can close this, as Deferred Vector is only used in lambdify for this purpose.

@jcrist

jcrist Jan 31, 2015

Member

Mmk. I'll try to get lambdify working with MatrixSymbol this week. Then we can close this, as Deferred Vector is only used in lambdify for this purpose.

This comment has been minimized.

@moorepants

moorepants Jan 31, 2015

Member

Ok, thanks. If you do I'll support it, but I'll have to support both unless we want to up the SymPy dependency version for PyDy.

@moorepants

moorepants Jan 31, 2015

Member

Ok, thanks. If you do I'll support it, but I'll have to support both unless we want to up the SymPy dependency version for PyDy.

pydy/codegen/ode_function_generator.py
+ @staticmethod
+ def _deduce_system_type(**kwargs):
+
+ if kwargs.pop('coordinate_derivatives') is not None:

This comment has been minimized.

@jcrist

jcrist Jan 31, 2015

Member

I'm a little confused by this. Are these user set kwargs? They appear to work as long as their defined, so I could pass in coordinate_derivatives=True or coordinate_derivatives=False, or coordinate_derivatives="yay duck typing", and get the same result each time?

What is the meaning of the different options? Can they contradict each other?

@jcrist

jcrist Jan 31, 2015

Member

I'm a little confused by this. Are these user set kwargs? They appear to work as long as their defined, so I could pass in coordinate_derivatives=True or coordinate_derivatives=False, or coordinate_derivatives="yay duck typing", and get the same result each time?

What is the meaning of the different options? Can they contradict each other?

This comment has been minimized.

@moorepants

moorepants Jan 31, 2015

Member

The generators support three versions of the EoMs:

  1. x' = f(x, t, r, p)
  2. M(x, p) * x' = f(x, t, r, p)
  3. M(q, p) * u' = f(q, u, t, r, p); q' = g(q, u, t)

This code just determines which of the above three you likely passed in. If you pass in garbage (not matrices), then you'll get an error. I could make explicit checks here for what each arg is supposed to be, but right now it is more or less "garbage in, garbage out".

@moorepants

moorepants Jan 31, 2015

Member

The generators support three versions of the EoMs:

  1. x' = f(x, t, r, p)
  2. M(x, p) * x' = f(x, t, r, p)
  3. M(q, p) * u' = f(q, u, t, r, p); q' = g(q, u, t)

This code just determines which of the above three you likely passed in. If you pass in garbage (not matrices), then you'll get an error. I could make explicit checks here for what each arg is supposed to be, but right now it is more or less "garbage in, garbage out".

This comment has been minimized.

@jcrist

jcrist Jan 31, 2015

Member

Ah, that makes sense. Just curious, looks fine to me as is.

@jcrist

jcrist Jan 31, 2015

Member

Ah, that makes sense. Just curious, looks fine to me as is.

pydy/codegen/ode_function_generator.py
+
+ args = self._parse_constants(*args)
+
+ if self.specifieds is not None:

This comment has been minimized.

@jcrist

jcrist Jan 31, 2015

Member

Since this is always checked, perhaps this check could be moved inside self._parse_specifieds?

@jcrist

jcrist Jan 31, 2015

Member

Since this is always checked, perhaps this check could be moved inside self._parse_specifieds?

This comment has been minimized.

@moorepants

moorepants Jan 31, 2015

Member

Yeah, I don't like how I've defined everything here so much. It also takes way too much time to run these parsing functions to support all the different args in each rhs eval. I'll clean it up.

@moorepants

moorepants Jan 31, 2015

Member

Yeah, I don't like how I've defined everything here so much. It also takes way too much time to run these parsing functions to support all the different args in each rhs eval. I'll clean it up.

moorepants added some commits Feb 4, 2015

Refactors the arg parsing the rhs to maximize speed.
- Removed the Mixins all together and pushed everything into
  ODEFunctionGenerator.
- Modularized the rhs arg parsing functions and made some minor speed ups with
  regards to array allocation.
- Introduced user options to specifiy apriori what type of rhs arguments they
  plan to use. This way the rhs function can be optimized for speed.
- Refactored the tests to check all the new stuff.

The fact that we have some many rhs extra arg options makes this pretty hairy.
This was the best solution I could come up with.
The rhs() docstring now reflects the user's specifications.
- Simplified arg handling in generate_ode_function.
- Reordered define_inputs.
Cleaned up docstrings and method names.
- A single docstring for the generators is present and the one for the
  functional interface is autogenerated.
- I made attributes private/public based on whether you use them in the sub
  class.
@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Feb 4, 2015

Member

@pydy/pydy-developers

Ok, I've addressed the original comments in the latest commits. I also re-factored significantly so that the rhs evaluation can be optimized for speed and the Mixins are gone. I'm ready to merge this. If anyone has any more comments, please make them in the next 24 hrs.

Thanks!

Member

moorepants commented Feb 4, 2015

@pydy/pydy-developers

Ok, I've addressed the original comments in the latest commits. I also re-factored significantly so that the rhs evaluation can be optimized for speed and the Mixins are gone. I'm ready to merge this. If anyone has any more comments, please make them in the next 24 hrs.

Thanks!

moorepants added a commit that referenced this pull request Feb 5, 2015

Merge pull request #113 from pydy/integration-speed
Adds more flexible code generators for rhs functions that result in faster eval times.

@moorepants moorepants merged commit 75f6fb5 into master Feb 5, 2015

1 check passed

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

@moorepants moorepants deleted the integration-speed branch Feb 5, 2015

@moorepants moorepants referenced this pull request in sympy/sympy Feb 6, 2015

Closed

SymPy has invalid version numbers for PEP 440 #8953

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