kwargs can be passed from System.generate_ode_function to the matrix generator #356
Conversation
…generator. This implements an _options attribute to the subclass of ODEFunctionGenerator and thus allows various options needed for a specific MatrixGenerator to be supplied all the way at the System.generate_ode_function and generate_ode_function levels.
| self._options = {'tmp_dir': None, | ||
| 'prefix': 'pydy_codegen', | ||
| 'cse': True, | ||
| 'verbose': False} |
There was a problem hiding this comment.
This _options dictionary could probably be built by inspecting the methods of the CythonMatrixGenerator, but I didn't try to figure that out.
There was a problem hiding this comment.
I'll be able to give it more of a look over later today or possibly tomorrow. First look though it appears that you are adding more things that a user can set. If this is the case should this be added to a docstring or documentation somewhere? If I remember right the docstring is built dynamically for this class and may be difficult to edit.
|
@jbm950, would you mind reviewing this? |
| 'verbose': False} | ||
| for k, v in self._options.items(): | ||
| self._options[k] = kwargs.pop(k, v) | ||
|
|
There was a problem hiding this comment.
This works as a way to pull the arguments out of kwargs properly but ends up setting each item in the self._options dictionary twice. Another way would be to just call kwargs for each item like this
self._options = {}
self._options['tmp_dir'] = kwargs.pop('tmp_dir', None)
self._options['prefix'] = kwargs.pop('prefix', 'pydy_codegen')
self._options['cse'] = kwargs.pop('cse', True)
self._options['False'] = kwargs.pop('verbose', False)This method probably a bit more verbose and thus readability is reduced but I imagine the performance would improve because each item in the dictionary is only being set once. What do you think?
There was a problem hiding this comment.
Actually, my code might fail if the options aren't in kwargs. I'll look at this again.
There was a problem hiding this comment.
Oh, no it is fine. Yeah, I could write it your way too. I'm not sure it really matters, as this is not a place that performance matters.
|
Looks like you added input support for four new arguments ( That, the documentation comments and the question on possibly increasing the efficiency are the only things I have. Other than that it looks like the functionality was extended well. |
All of the options are tested in the CythonMatrixGeneratorTests. All that is needed to test here is whether the arguments are passed in from
The docstring of system says that any extra kwargs will be passed through to the underlying routines and those routines have clear docstrings about their optional arguments. I don't think added all of the options for the underlying generators is necessary in System's docstrings. But I'll look into adding something if there is a good spot for it. |
|
That works for me then. What did you think about building the options dictionary one item at a time and getting rid of the call to |
In that case readability > performance and I think the way you have it currently written makes it easier to see at a glance what is supposed to be in the I'll go ahead and merge this unless you have any objections. |
|
FYI, for next time. Please review the list at the top of the PR and check off things that have been taken care of before merging. |
This implements an _options attribute to the subclass of ODEFunctionGenerator
and thus allows various options needed for a specific MatrixGenerator to be
supplied all the way at the System.generate_ode_function and
generate_ode_function levels.
commit message.
nosetests) and on Travis CI.format.)
docsdirectory)
pylint, to check your code)
Notes.
follow deprecation cycles.)
@asmeurer This fixes the issue you had this morning.