-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds a NumPy backend so opty can be used without Cython (and thus C compilation). #372
Conversation
Timing comparison for the one legged time trial example: NumPy backend:
Cython backend:
|
Numpy looks much faster or am I reading it wrong? |
NumPy is 120x slower for this example. |
I mixed up millisec and microsec! |
Do you have any thoughts on this new api? |
@@ -269,7 +269,7 @@ def test_ufuncify_matrix(): | |||
|
|||
a_vals = np.random.random(n) | |||
b_vals = np.random.random(n) | |||
c_vals = np.random.random(n) | |||
c_vals = np.abs(np.random.random(n)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still get a warning for a pow() when evaluating. I'm not sure what number is causing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opty/tests/test_utils.py::test_ufuncify_matrix
/home/moorepants/src/opty/opty/tests/test_utils.py:283: RuntimeWarning: invalid value encountered in power
result[:, 0, 0] = a_vals**2 * np.cos(np.pi*b_vals)**c_vals
opty/tests/test_utils.py::test_ufuncify_matrix
<lambdifygenerated-25>:4: RuntimeWarning: invalid value encountered in scalar power
return array([[x0*cos(pi*b)**_Dummy_250, _Dummy_250**4 + tan(b)/sin(x1)], [-sqrt(_Dummy_250) + b**2 + x0, x1*(_Dummy_250 + x1)*sin(b)/a]])
I assume for a computer idiot like me, numpy is better: one less 'thing' to mess around with. |
I am implementing this so you and others can use opty without having to install a C compiler and ensure the compiler is working properly with Python (an issue that you and others have faced). So, yes I'm trying to make the software simpler to use. Yes, the conversation with Aaron points to the fact that I can't harness the full speed of NumPy when using SymPy's lambdify for this specific problem. NumPy will actually run these computations faster than what is shown here, but currently not via SymPy's lambdify code generation. The fact that I have to have a loop through all nodes in Python code, is the reason it is slow. But, I'm not really asking you about either of those points. I'm curious if you think how I've added this feature is appropriate for opty. Did I design the API well? Should I use different variable names? Should I have different documentation? i.e can your review these code changes because it is good that we feel pretty confident that we wouldn't need to change any of the API later (causing backwards incompatibilities). |
Also, do I have enough tests? Does it cover all cases? etc. |
This comment was marked as duplicate.
This comment was marked as duplicate.
So that I understand correctly: |
I'm asking you to look at all the changes I made in this PR and give specific feedback on the changes. |
Clear! Already started. In line 586 you write function in direct_colocation.py Dumb question: |
For opty, I do not think there is a reason a user would ever not want cse, so it it has never been an option for them to say anything about that. Before the addition of this function, there was no choice at all to not have cse. I actually only added it to Note that you can press the blue "+" symbol on any line in the files changes tab to add a comment directly at that line. |
Also, thanks for the very helpful feedback. Exactly what I was hoping for! |
shown. Only available with the ``'cython'`` backend. | ||
backend : string, optional | ||
Backend used to generate the numerical functions, either | ||
``'cython'`` (default) or ``'numpy'``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The capitalization here is because "backend" is the first word in the sentence. Just above it is lowercase because that is the exact case-sensitive kwarg. @Peter230655 can you clarify you comment on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The capitalization here is because "backend" is the first word in the sentence. Just above it is lowercase because that is the exact case-sensitive kwarg. @Peter230655 can you clarify you comment on this?
What I meant was this: backend
, with small b is the name of the kwarg. Hence, no matter where it stands in a sentence its name should be spelled the same way. But a VERY minor point indeed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using "Backend" in the sentence as normal English word, like "The backend of the government's computer system is very complex."
I totally agree regardind cse=True always: You may remember that I wotze some sympy mechanics simulations with rather large eoms. Only after I learned about cse=True to be used in lambdify would solve_ivp get a result in good time. I did not know about this blue "+". Let me try it. |
I think I'm good with this, so merging. We can tweak before a next release if needed. |
This is a first shot at fixing #367 but I quickly hit a limitation of trying to have the loop evaluation purely as a NumPy vectorized operation. See the following error:
and then the SymPy issues that explain why this can't be done: sympy/sympy#27632
I will change the code to function in the next commits, but the performance will likely suffer.