Prototype System class. #81

chrisdembia · 2014-07-29T16:54:57Z

@moorepants, can add your tests to this branch? From this point I'll write code in a test-driven way. It'd be fun to continue to work on this together, but I'm also happy to do the coding and you can review afterwards (if you're busy, etc.).

- Edited double_pendulum example to use System class. - Started writing the system class. This commit is partial. Maybe we can later squash it with other commits.

chrisdembia · 2014-07-29T16:55:39Z

@tarzzz this will allow for re-integrating (though, as Jason and I discussed, it's not necessary for re-integrating).

moorepants · 2014-07-29T17:06:45Z

I added what I started. The assert_raises are bogus, they need to be correctly done. Feel free to run with things. I'll help when I can.

moorepants · 2014-07-29T17:12:22Z

pydy/codegen/tests/models.py

@@ -9,7 +9,7 @@
 import sympy.physics.mechanics as me


-def generate_mass_spring_damper_equations_of_motion(external_force=True):
+def generate_mass_spring_damper_equations_of_motion(external_force=True, kane=False):


We should just have these models output a System in the future.

chrisdembia · 2014-07-29T17:21:56Z

In sympy/sympy#7790, @jcrist said:

I've been thinking about our type system in mechanics, and have some thoughts that would make more sense in hindsight but would be a pain to change due to deprecation. The main one is that KanesMethod and LagrangesMethod are basically factories for the EOM of a system. Further, they each seem to support rederivation using different forces/coordinates/etc..., which then mutates their respected attributes (mass and forcing matrices).

What would make more sense to me is to have a system class that holds the generalized system form (call it EOM or something). This would just store the results, not the method that derived them, and would implement the mass and forcing matrix stuff. With it in this form as a storage method, to_linearizer and linearize would be simpler too. And I imagine the visualization stuff as well. KanesMethod and LagrangesMethod would then act more as functions that return a derived system. Just some thoughts.

chrisdembia · 2014-07-29T17:24:12Z

@jcrist Look at how we are thinking one would construct a System. Are you instead proposing a constructor that would be called like System(KM.equations()) or something like that? If you think such a class would be beneficial to your work, let's make sure we design it with those considerations.

chrisdembia · 2014-07-29T17:26:53Z

@moorepants and I discussed this for a while last night, about having a class that just held equations. We discussed that then the class would just be a mathematical thing (like EOM). We were discussing that maybe we don't want to lose information about the mechanics (bodies, etc), since those would be useful for setting up visualization.

@moorepants how concerned should we be about deprecation?

I also want to have a functional version of this class by the end of the week; I don't want this to explode into a big project. We can have bigger plans, but I think we want something functional and simple for now.

jcrist · 2014-07-29T17:29:45Z

pydy/system.py

+        values. If not provided, we get all the specified symbols
+        from the `method`, and set their numerical value to 0. You can also
+        provide a subset of symbols for this attribute, in which case the
+        remaining symbols still have their numerical value set to 0.


What's the difference between the constants dictionary and specified_symbols/specified_values? If I were to do: dict(zip(specified_symbols, specified_values)) would this be the same as constants?

Oh. Reading further on in the code I can see these are dynamicsymbols, not symbols. Perhaps a better name for these parameters? fixed_dynsyms? I think a dictionary for specifying these would be cleaner as well, as they are relational (can't have one iterable longer than the other). If a user has iterables, could just run dict(zip(*)) to make the dict, and then pass it to the method themselves.

Thanks @jcrist. This is something that @moorepants and I discussed for a bit but we are still not completely satisfied. We agree with your point about how they are relational. However, Jason was saying that the specified_values could be a function that returns more than one number, to define multiple specified_symbols. That is, you may have symbols [a, b, c] and values lambda x, t: np.zeros(3) to define all 3 of those symbols. If we were to use a dict, we'd have to give each symbol its value separately.

Can you think of a good solution that (1) allows simulatenously defining a numerical value for more than one specified symbol at a time, and (2) enforces the relational (key, value) aspect?

And yeah good point, the documentation for distinguishing symbols and specified's could be better. specifieds is also kinda weird b/c it's an adjective, not a noun.

That sounds like something more complicated that should be done outside the class. Suppose I have an iterable of symbols: [a, b, c, d, e, f], and an iterable of functions [f1, f2]. f1 generates the value for a, b, and f2 generates for c - f. What if the code for f1 changed to now return 3 things? Now you have an off by one error all symbols after b. I'm not sure how you can resolve this in a robust way with multiple returns from a single function. Unless you plan on dynamically calling these functions in a systematic manner during integration/simulation, I don't think this use is necessary. The user can still just as easily call the functions themselves, and generate the dictionary to pass to System.

That may work. I like it.

Okay I want to go that route but it may cause issues when passing self.specifieds.keys() to pydy.codegen.code.generate_ode_function. Does anyone have insight as to what'll happen here?

@moorepants I don't know enough about how generate_ode_function parses the list of functions.

We can update generate_ode_function too with this I guess. I guess generate_ode_function could take a system as an argument now.

Right now the rhs function requires the specified to be in a certain form. This shows what it needs to be: https://github.com/pydy/pydy/blob/master/pydy/codegen/code.py#L441

Ah shucks. What I have now does not conform. I'll commit it and maybe we can work on making it conform.

jcrist · 2014-07-29T17:43:22Z

Losing information about the bodies would be an issue. I'm not sure how visualization works now, but some way of relating a rigid body in space to the dynamicsymbols could probably be found that didn't rely on housing inside the *Method classes. As it is they just hold a body_list and a force_list. I was actually thinking that we could change the *Method class to return a System object. But again, this is a major change, and it sounds like you just want something that works. As it is I think the layout looks fine.

chrisdembia · 2014-07-29T17:53:16Z

In the future, I want everything, so we can keep discussing. But for this week, I just want whatever will help out you, @tarzzz , and @moorepants .

chrisdembia · 2014-07-29T17:53:51Z

I hadn't thought of *Method returning a System...hmm. I'd be interested in hearing more about that.

moorepants · 2014-07-29T17:56:40Z

@chrisdembia Deprecating things in SymPy releases needs to be adhered too but here, in PyDy, this is alpha software, so we can change up things. But...this stuff is in the wild now and I just gave 4 tutorials on it so people may use it. I'd like to minimize users getting confused and annoyed if possible. It can have a detrimental effect on retaining users. But like I said, this is classified as alpha software.

moorepants · 2014-07-29T17:59:45Z

One classic example for specified inputs are controllers. State feedback controllers are computed like f = -k * x, and if x is big then you have a big matrix multiplication. You only want to call this one per integration step to get the n inputs. So it doesn't really work with the dictionary concept. Maybe we need to support both methods of input: dictionary and function. The bottom line is that we don't want to cripple the speed of any computation that must happen at every time step. Computation of speified inputs is one of those.

jcrist · 2014-07-29T18:03:08Z

Oh, they're the inputs! This was not clear to me above. Yeah, dynamically solving for them is very important in simulation. I think the dictionary concept I outlined above retains flexibility, while still being computationally simple enough to work.

moorepants · 2014-07-29T18:09:28Z

BTW, after reading Jim's comment on a super class for the *Methods, I think that makes sense to have one. It can be an abstract one for sure, but it would at least make sure we conform on attribute names for the things we need in the system class.

chrisdembia · 2014-07-29T18:15:24Z

It'd be nice if _q and _u were not hidden, but were instead maybe read-only?

moorepants · 2014-07-29T18:22:54Z

Agreed.

tarzzz · 2014-07-29T18:24:44Z

examples/double_pendulum/simulate.py


 frames_per_sec = 60
 final_time = 5.0

 t = linspace(0.0, final_time, final_time * frames_per_sec)
-x = odeint(xdot_function, x0, t, args=(args,))
+
+x = sys.integrate(times)


This is what I had been implying for re-integrating Scene visualization stuff.
So that we supply times and sys to the Scene class, and is easier to handle than the existing structure.
I am ready to work on integrating this code with Scene class(after tomorrow).

But also there will be some backwards incompatible changes(in Scene class as well), when this is integrated into Scene.

Yes, that is why we are creating this, so that you can have something easier to work with in your viz code. It won't be ready tomorrow, but please help contribute to this so we can get it done more quickly.

chrisdembia · 2014-07-30T17:15:09Z

When using KanesMethod, the user must call kanes_equations to actually populate mass_matrix, etc. Is there similar behavior for LagrangesMethod? @jcrist, do you know?

chrisdembia · 2014-07-30T17:16:40Z

Ah I think the answer is yes, and the method is form_lagranges_equations. @jcrist can you confirm?

I didn't run any tests or check for syntax errors :/ specifieds need a lot of work.

chrisdembia · 2014-07-30T18:21:28Z

Okay we're getting close. I'd like some help. Please see the initial message to see where you might be able to help.

moorepants · 2014-07-31T03:57:39Z

pydy/system.py

+        km = KanesMethod(...)
+        km.kanes_equations(force_list, body_list)
+        sys = System(km)
+        times = [0, 5.0]


Shouldn't we pass in a vector of times instead of a start and stop? Some people may want variable step outputs or to control the time step.

times can be either I think; it's whatever odeint can accept. I was showing the start and stop for simplicity. Maybe the example is misleading.

I think odeint only accepts a vector of times: t : array

A sequence of time points for which to solve for y. The initial value point should be the first element of this sequence.

tarzzz · 2014-08-01T07:57:28Z

Should not system be made into an independent submodule? It is fine to put it here, since it is in development, but later on we can migrate it such that dir. structure looks like:

pydy/
-----viz
-----codegen
-----system

tarzzz · 2014-08-01T09:05:10Z

examples/double_pendulum/visualize.py

@@ -51,14 +51,13 @@
 # this, we create a world frame that is rotated +90 degrees about the N frame's
 # z direction.
 world_frame = N.orientnew('world', 'Axis', [0.5 * pi, N.z])
-scene = Scene(world_frame, O,
+scene = Scene(sys, world_frame, O,
        linkP_viz_frame, linkR_viz_frame, sphereP_viz_frame, sphereR_viz_frame)


Is there any way, that we can use System to eliminate the ReferenceFrame and Point argument supplied here.
Maybe they can be saved in System class too?

so that all we need to do is:

scene = Scene(sys, vframes_as_a_list_or_tuple)

That would be nice to clean up the Scene constructor. However, I am not sure this is a good idea. Those two arguments are specific to how the visualization looks, and they have been useful for me before. We can consider this again down the line when System becomes bigger and potentially manages the viz info. But for now (this PR), System does not handle viz info.

I think you have to have the option of providing any frame or point as the base viz frame. You could snatch the base frame from the Methods as a default frame, and maybe you could go through all the points stored in the Methods class and find one that has zero velocity in the base frame as a default.

moorepants · 2014-08-05T17:57:28Z

pydy/system.py

+    def _Kane_inlist_insyms(self):
+        """TODO temporary."""
+        uaux = self.eom_method._uaux
+        uauxdot = [diff(i, t) for i in uaux]


Seems like this should fail as diff isn't imported anywhere.

moorepants · 2014-08-05T18:15:23Z

I just realized that moving the specified parsing out of the rhs isn't that trivial because of the function of x and t. So I guess we will keep this as is and just note the backward incompatibility.

moorepants · 2014-08-05T18:25:48Z

I just did a speed test on evaluating the rhs in master and this branch. The old version gave me 189 us per rhs eval and the new gave 380 us per eval. That's a significant increase for the dictionary parsing. Here is a profile:

In [16]: %prun for i in range(1000): rhs(np.ones(8), 0.0, a)
         278003 function calls in 0.471 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    12000    0.090    0.000    0.132    0.000 function.py:396(class_key)
     6000    0.054    0.000    0.203    0.000 basic.py:333(__eq__)
     1000    0.045    0.000    0.076    0.000 <string>:1(<lambda>)
    12000    0.040    0.000    0.040    0.000 function.py:129(nargs)
     1000    0.031    0.000    0.516    0.001 code.py:413(evaluate_ode)
    10000    0.029    0.000    0.029    0.000 {numpy.core.multiarray.array}
     1000    0.027    0.000    0.059    0.000 linalg.py:296(solve)
    52000    0.017    0.000    0.017    0.000 {isinstance}
     1000    0.013    0.000    0.195    0.000 code.py:356(mass_forcing_func)
     2000    0.010    0.000    0.040    0.000 defmatrix.py:244(__new__)
     4000    0.008    0.000    0.018    0.000 shape_base.py:8(atleast_1d)
     2000    0.008    0.000    0.012    0.000 {built-in method __new__ of type object at 0x7ff2befcec40}
     4000    0.008    0.000    0.211    0.000 {method 'index' of 'list' objects}
    44000    0.007    0.000    0.007    0.000 {math.sin}
     2000    0.007    0.000    0.007    0.000 {numpy.core.multiarray.copyto}

moorepants · 2014-08-05T18:29:33Z

This speed decrease is because we want to support this fancy dictionary for specified inputs. I'm not sure it is worth it. It is a really nice convenience, but adds some overhead. Should we worry about it?

chrisdembia · 2014-08-05T18:56:56Z

I think that's too much of a speed decrease. I'm not sure what we should
do...

On Tue, Aug 5, 2014 at 11:29 AM, Jason K. Moore notifications@github.com
wrote:

This speed decrease is because we want to support this fancy dictionary
for specified inputs. I'm not sure it is worth it. It is a really nice
convenience, but adds some overhead. Should we worry about it?

—
Reply to this email directly or view it on GitHub
#81 (comment).

chrisdembia · 2014-08-06T20:09:31Z

My recommendation is to leave as-is now and create a new issue for restoring the previous efficiency by supplying a different value for System.specifieds. I'd suggest the options are:

The dict as it was previously.
A dict with just two keys: 'symbols' and 'values'. symbols will be an iterable of the symbols in the correct order. and values will be what codegen used to accept: a list of values or a single function.

chrisdembia · 2014-08-07T16:42:34Z

Ready for final review. @moorepants let me know when this can be merged. I allowed users a fast path (3x faster using 'symbols' and 'values', restoring the previous speed).

However, there is a small complication. If users, with the same System instance, switch between formats for specifieds, they'll need to re-call generate_ode_function. That's b/c they are in control of the order of specifieds under 'symbols' and 'values' format.

moorepants · 2014-08-07T16:51:52Z

One worry here. If we push as is, and Tarun uses it for the Scene stuff, then you will only be able to visualize scenes created from KanesMethod. So we will, at least, need to make an release stopping issue to fix that too.

Would a Method abstract base class be relatively simple? We just need to support mass_matrix_full, forcing_vector_full, coordinates, speeds, specified, etc. Maybe that wouldn't be too hard.

moorepants · 2014-08-07T16:53:30Z

README.rst

+
+   sys = System(kane,
+        constants={mass: 1.0, stiffness: 1.0, damping: 0.2, gravity: 9.8},
+        specified={'symbols': [force], 'values': lambda x, t: sin(t)},


How do you make sure that your list of specifieds is the same order as those found from the kane object?

I don't. If the user is using the symbols/values format, they are in control of the order of symbols. See https://github.com/pydy/pydy/pull/81/files#diff-105304ef1b8479ef4d5d9e41b786cc34R383

moorepants · 2014-08-07T16:54:39Z

I'm going to do an editing pass here over every thing. So maybe hold off editing.

chrisdembia · 2014-08-07T17:50:08Z

One worry here. If we push as is, and Tarun uses it for the Scene stuff, then you will only be able to visualize scenes created from KanesMethod. So we will, at least, need to make an release stopping issue to fix that too.

Ahh good point!

Would a Method abstract base class be relatively simple? We just need to support mass_matrix_full, forcing_vector_full, coordinates, speeds, specified, etc. Maybe that wouldn't be too hard.

The thing I don't know how to do is find the list of constants and specifieds for a LagrangesMethod. That's the limitation (for me).

moorepants · 2014-08-07T18:39:33Z

@jcrist has some new methods in sympys sympy/sympy#7790 that adds generic functions for finding the various symbols for either method. We need to use those.

chrisdembia · 2014-08-07T20:27:11Z

So should that get in first?

moorepants · 2014-08-07T21:19:31Z

He's about to merge those. So we can work with SymPy master for PyDy master or just copy his code to pydy temporarily until sympy makes a new release.

chrisdembia · 2014-08-07T21:51:56Z

Both sound messy! I guess the latter makes more sense. Is it okay if the visualizer temporarily does not support Lagrange (until the next sympy release)? Alternatively, the visualizer can for now just have partial support of LagrangesMethod systems.

chrisdembia · 2014-08-07T21:52:52Z

That would require some effort on @tarzzz 's behalf so that Scene can only optionally work with a System (an alternate constructor that just uses the System to initialize current constructor arguments).

moorepants · 2014-08-07T21:57:46Z

I'm fine merging this as is, but make an issue for the 0.3.0 milestone that requires us to support KanesMethod, LangrangesMethod, and some kind of generic method for when a user builds the model manually not using a method class.

chrisdembia · 2014-08-07T22:03:04Z

Should I merge?

moorepants · 2014-08-07T22:06:06Z

Sure

Prototype System class.

chrisdembia · 2014-08-07T22:09:34Z

@tarzzz feel free to start using this.

Prototype System class.

77cd5d4

- Edited double_pendulum example to use System class. - Started writing the system class. This commit is partial. Maybe we can later squash it with other commits.

chrisdembia mentioned this pull request Jul 29, 2014

Mechanics improvements sympy/sympy#7790

Merged

WIP: Started tests for System class.

61cd1a7

moorepants reviewed Jul 29, 2014
View reviewed changes

jcrist reviewed Jul 29, 2014
View reviewed changes

tarzzz reviewed Jul 29, 2014
View reviewed changes

Removing default behaviors from System class.

29fc9cb

chrisdembia added 2 commits July 30, 2014 11:18

Fix bug with mass spring damper kwarg.

418c1fc

WIP Write most of the System class, with tests.

2a5b721

I didn't run any tests or check for syntax errors :/ specifieds need a lot of work.

moorepants reviewed Jul 31, 2014
View reviewed changes

tarzzz reviewed Aug 1, 2014
View reviewed changes

moorepants reviewed Aug 5, 2014
View reviewed changes

moorepants added the enhancement label Aug 5, 2014

Restore the fast way of parsing specifieds.

fd981e7

moorepants reviewed Aug 7, 2014
View reviewed changes

Edits to the README.

f6c03a3

chrisdembia mentioned this pull request Aug 7, 2014

Create Method abstract base class, with a UserDefinedMethod class. #83

Open

moorepants added this to the 0.3.0 Release milestone Aug 7, 2014

chrisdembia added a commit that referenced this pull request Aug 7, 2014

Merge pull request #81 from pydy/system

64732fd

Prototype System class.

chrisdembia merged commit 64732fd into master Aug 7, 2014

chrisdembia deleted the system branch August 7, 2014 22:09

moorepants mentioned this pull request Aug 7, 2014

All examples should output a System class #84

Open

Prototype System class. #81

Prototype System class. #81

Conversation

chrisdembia commented Jul 29, 2014

chrisdembia commented Jul 29, 2014

moorepants commented Jul 29, 2014

Choose a reason for hiding this comment

chrisdembia commented Jul 29, 2014

chrisdembia commented Jul 29, 2014

chrisdembia commented Jul 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrist commented Jul 29, 2014

chrisdembia commented Jul 29, 2014

chrisdembia commented Jul 29, 2014

moorepants commented Jul 29, 2014

moorepants commented Jul 29, 2014

jcrist commented Jul 29, 2014

moorepants commented Jul 29, 2014

chrisdembia commented Jul 29, 2014

moorepants commented Jul 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdembia commented Jul 30, 2014

chrisdembia commented Jul 30, 2014

chrisdembia commented Jul 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarzzz commented Aug 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorepants commented Aug 5, 2014

moorepants commented Aug 5, 2014

moorepants commented Aug 5, 2014

chrisdembia commented Aug 5, 2014

chrisdembia commented Aug 6, 2014

chrisdembia commented Aug 7, 2014

moorepants commented Aug 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorepants commented Aug 7, 2014

chrisdembia commented Aug 7, 2014

moorepants commented Aug 7, 2014

chrisdembia commented Aug 7, 2014

moorepants commented Aug 7, 2014

chrisdembia commented Aug 7, 2014

chrisdembia commented Aug 7, 2014

moorepants commented Aug 7, 2014

chrisdembia commented Aug 7, 2014

moorepants commented Aug 7, 2014

chrisdembia commented Aug 7, 2014