modifications in _create_rhs_function

[yashu-seth]

• There are no merge conflicts.
• If there is a related issue, a reference to that issue is in the
  commit message.
• Unit tests have been added for the new feature.
• The PR passes tests both locally (run nosetests) and on Travis CI.
• All public methods and classes have docstrings. (We use the numpydoc
  format.)
• An explanation has been added to the online documentation. (docs
  directory)
• The code follows PEP8 guidelines. (use a linter, e.g.
  pylint, to check your code)
• The new feature is documented in the Release
  Notes.
• The code is backwards compatible. (All public methods/classes must
  follow deprecation cycles.)
• All reviewer comments have been addressed.

[yashu-seth]

@moorepants The changes are not complete. But this should give you an idea. Please tell if I am on the right track.

[moorepants]

Please describe what you did in the PR description and you'll need to show that this does not increase execution speed of the right hand side function.

[yashu-seth]

@moorepants Instead of describing each possible permutation I have assigned r and p values as functions for each case. This shall allow writing the rhs function only once, where it can take the appropriate conversions for args[-1] and args[-2] from the assigned r and p values. The complexity remains the same i.e. O(m+n), where m is the number of permuations and n is the number of times we call the rhs function. Only overhead shall be in calling the two r and p functions. But it can also save some checks that must be done in the rhs function in the former case.

[yashu-seth]

An example -

def foo1(x,y):
    if x>y:
        a = lambda li:li[-1]    
    else:
        a = lambda li:sum(li)
    def rhs(li):
        return (a(li))
    return rhs

k1= foo1(5,3)


def foo2(x,y):
    if x>y:
        def rhs(li):
            return (li[-1])    
    else:
        def rhs(li):
            return sum(li)
    return rhs

k2 = foo2(5,3)

foo1 is what I am proposing. foo2 is how things have been implemented currently.

[yashu-seth]

@moorepants I also feel that functions like _parse_constants and _parse_specifieds should take p and r values respectively instead of *args. It can save a lot of unnecessary parsing.

[oliverlee]

@yashu-seth It would be helpful to provide more descriptive git commit messages instead of only a summary line. But maybe you were planning on doing that later as you mentioned that this PR is still a work in progress.

A good reference for writing git commit messages can be found here: https://git-scm.com/book/ch5-2.html

[moorepants]

We just pushed some fixes so that the Travis builds work again. Please update with the master branch so that we can see if the tests pass.

[yashu-seth]

@oliverlee thanks for the update. I'll be careful with the commit messages, although yes the commits are not final. I am still working on the PR.

[yashu-seth]

@moorepants okay. Meanwhile do you have any suggestions related to the changes in the PR?

[moorepants]

I still haven't digested it completely as I haven't looked at this code in a while. My only concerns are that the public API stays the same and that the generated rhs function executes as fast as possible for the different types of input. if you change slows things down for any of the input types, then I wouldn't want to make the change. So please show some the timing differences comparing your PR to the current code.

[yashu-seth]

@moorepants the timing difference will increase marginally for my PR. As there there are some extra function calls of O(1) . But it can save us from the former messy implementation. And yes, the complexity remains the same.

[yashu-seth]

@moorepants Okay let me complete the changes, and then I'll show you the timing differences.

[moorepants]

The marginal time increase can be a big deal when you are evaluating the rhs millions of times.

[moorepants]

It looks like you are getting test failures now.

[yashu-seth]

@moorepants okay let me complete the changes, then you can decide If the changes are affordable or not.

[yashu-seth]

@moorepants Can you please suggest me something which can help me in showing the time difference for the PR changes?

[yashu-seth]

@moorepants I think there are few small changes that could still be done. I'm looking into them.

[moorepants]

You can use the pendulum model to generate a rhs function for the different backends and input types, then use timeit() to compare the execution before and after this change. If you write up a script we can include it in the repository and use it for future timing comparisons.

Another option would be to setup benchmarking like we did for sympy (will take some more work): https://github.com/sympy/sympy_benchmarks. Regardless you need to write some scripts to test things. The current unit tests can likely be modified slightly to do this.

[yashu-seth]

thank you. I'll look into it.

[yashu-seth]

@moorepants Can you please look into this now? I did not find a significant time difference. It keeps varying.

[moorepants]

Can you post the timings from master and the timings from this branch here?

[yashu-seth]

@moorepants For this PR -

The time taken by rhs function in 1000 iterations
For constants argument type - "None" and specifieds argument type - "None" is 
7.188482653975492
For constants argument type - "None" and specifieds argument type - "array" is 
6.6111639599025105
For constants argument type - "None" and specifieds argument type - "function" is 
6.740118141617553
For constants argument type - "None" and specifieds argument type - "dictionary" is 
6.662771115699357
For constants argument type - "array" and specifieds argument type - "None" is 
6.695045161458864
For constants argument type - "array" and specifieds argument type - "array" is 
6.443426343266893
For constants argument type - "array" and specifieds argument type - "function" is 
6.71571419108227
For constants argument type - "array" and specifieds argument type - "dictionary" is 
6.758470397575707
For constants argument type - "dictionary" and specifieds argument type - "None" is 
7.156479486779972
For constants argument type - "dictionary" and specifieds argument type - "array" is 
6.788033636213517
For constants argument type - "dictionary" and specifieds argument type - "function" is 
6.463170622706589
For constants argument type - "dictionary" and specifieds argument type - "dictionary" is 
6.721162862082366

[yashu-seth]

For master -

The time taken by rhs function in 1000 iterations
For constants argument type - "None" and specifieds argument type - "None" is 
7.119915856596703
For constants argument type - "None" and specifieds argument type - "array" is 
6.667910655479475
For constants argument type - "None" and specifieds argument type - "function" is 
6.62939567334471
For constants argument type - "None" and specifieds argument type - "dictionary" is 
6.9335815713009294
For constants argument type - "array" and specifieds argument type - "None" is 
6.502557019004731
For constants argument type - "array" and specifieds argument type - "array" is 
6.542005925740706
For constants argument type - "array" and specifieds argument type - "function" is 
6.515386431129836
For constants argument type - "array" and specifieds argument type - "dictionary" is 
6.761988397938343
For constants argument type - "dictionary" and specifieds argument type - "None" is 
6.500507080939816
For constants argument type - "dictionary" and specifieds argument type - "array" is 
6.482216713425099
For constants argument type - "dictionary" and specifieds argument type - "function" is 
6.5054377860979855
For constants argument type - "dictionary" and specifieds argument type - "dictionary" is 
6.748779413726727

[yashu-seth]

I must tell you that the time differences have been erratic, and sometimes both positive and negative for different executions of the script.

[moorepants]

I didn't look close enough earlier, but you have random choices happening everytime you run this script. The randomness needs to be removed.

[moorepants]

The random initial conditions should be changed to specific ones too.

[yashu-seth]

okay. i'll fix it.

[yashu-seth]

@moorepants for master -

The time taken by rhs function in 1000 iterations
For constants argument type - "array" and specifieds argument type - "array" is 
7.318102951750426
For constants argument type - "array" and specifieds argument type - "function" is 
7.167084026860331
For constants argument type - "array" and specifieds argument type - "dictionary" is 
7.583084067912154
For constants argument type - "dictionary" and specifieds argument type - "array" is 
7.104953337761955
For constants argument type - "dictionary" and specifieds argument type - "function" is 
7.212749203221449
For constants argument type - "dictionary" and specifieds argument type - "dictionary" is 
7.568671301790611

[yashu-seth]

for this PR -

The time taken by rhs function in 1000 iterations
For constants argument type - "array" and specifieds argument type - "array" is 
7.2501855106971105
For constants argument type - "array" and specifieds argument type - "function" is 
7.208652106953681
For constants argument type - "array" and specifieds argument type - "dictionary" is 
7.471582497990951
For constants argument type - "dictionary" and specifieds argument type - "array" is 
7.093712757911182
For constants argument type - "dictionary" and specifieds argument type - "function" is 
7.146613162458233
For constants argument type - "dictionary" and specifieds argument type - "dictionary" is 
7.448777122192619

[moorepants]

I just ran it with 10000 iterations to try to get a better time difference.

(pydy-examples-dev)moorepants@garuda:pydy(master)$ python bin/time_rhs.py 
The time taken by rhs function in 10000 iterations
For constants argument type - "array" and specifieds argument type - "array" is 
33.00295300100697
For constants argument type - "array" and specifieds argument type - "function" is 
33.46314385933025
For constants argument type - "array" and specifieds argument type - "dictionary" is 
35.409262089019954
For constants argument type - "dictionary" and specifieds argument type - "array" is 
33.88730369665427
For constants argument type - "dictionary" and specifieds argument type - "function" is 
32.87284890232453
For constants argument type - "dictionary" and specifieds argument type - "dictionary" is 
36.405540396342985
(pydy-examples-dev)moorepants@garuda:pydy(master)$ git pr 301
remote: Counting objects: 24, done.
remote: Compressing objects: 100% (13/13), done.
remote: Total 24 (delta 11), reused 5 (delta 5), pack-reused 6
Unpacking objects: 100% (24/24), done.
From github.com:pydy/pydy
 * [new ref]         refs/pull/301/head -> pr-301
M   bin/time_rhs.py
Switched to branch 'pr-301'
(pydy-examples-dev)moorepants@garuda:pydy(pr-301)$ python bin/time_rhs.py 
The time taken by rhs function in 10000 iterations
For constants argument type - "array" and specifieds argument type - "array" is 
33.95233248300307
For constants argument type - "array" and specifieds argument type - "function" is 
33.91573896134893
For constants argument type - "array" and specifieds argument type - "dictionary" is 
36.128026856652774
For constants argument type - "dictionary" and specifieds argument type - "array" is 
34.50351402235295
For constants argument type - "dictionary" and specifieds argument type - "function" is 
34.93311869333653
For constants argument type - "dictionary" and specifieds argument type - "dictionary" is 
34.78640400598912

Here they all have from .5 to ~1 second increase over the ~30 seconds, except for the last one that seems to be lower.

To be honest, I'm not sure that making the code shorter is worth the tradeoff from the performance overhead. The way this is all designed, we are tied to these Python callbacks which have performance overhead. The way I wrote it before tried to minimize the Python function calls as much as possible for the different arg types, which allowed the one with pure arrays to have as little Python overhead as possible.

@pydy/pydy-developers Does anyone else think that we should give up this bit of performance for code simplification?

[yashu-seth]

No problem at all. Lets see what others feel about this.

[oliverlee]

I don't expect python code I'm working with to be fast and feel that the improvement in code readability is worth the performance penalty.
+1

[moorepants]

Ok, I'm merging this. We can revisit it if there are future issues. My vision is to create just enough low level pieces such that the Python calls are as fast as anything you'd do in a lower level language. With some careful thinking and planning we can get C speeds on the rhs evaluation and integration, even in Python. For things like this, I do expect Python to be as fast, otherwise there isn't a good reason to use Python. For example, the research problems I use this on are big and require top speeds but I'd much rather code it in Python to save my development time.

[moorepants]

@yashu-seth Please add a note in the release notes and mention that there may be some performance degradation.

[yashu-seth]

@moorepants Ok. I'll add a release note.

[moorepants]

I added the release note, as it is simpler than you sending an extra PR.

[yashu-seth]

Ok.