-
Notifications
You must be signed in to change notification settings - Fork 0
Adds prod and hstack/vstack and fixes an issue with the newest version of NumPy #120
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
base: master
Are you sure you want to change the base?
Conversation
|
Benchmarks that have stayed the same: |
|
The new NumPy version breaks our tests. Because of this expired deprecation: https://numpy.org/doc/stable/release/2.4.0-notes.html#raise-typeerror-on-attempt-to-convert-array-with-ndim-0-to-scalar I can't tell if it is in IPOPT or us. |
|
The bug was on our end, we were returning a |
very nice fix, thanks for doing that! |
Transurgeon
left a comment
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 think this is looking good.
I only looked at the implementation of hstack and it makes a lot of sense.
I also double-checked all tests and they also seem to be correct.
The implementation of vstack is a bit confusing, but that's normal since we are considering fortran order.
|
Awesome, thank you for doing this! Two of the thoughts that popped up as I skimmed through hstack and vstack.
I think we should be careful so we are not sloppy and put unnecessary AI generated code in the repo. |
Actually, we don't even need to compute arg_size in such a complex way,
Fair point!
Ah there cannot be duplicates in either Jacobian. There can be in the Hessians (e.g. |
| Since prod is a smooth function with implemented gradients, | ||
| we simply ensure the argument is a Variable. | ||
| """ | ||
| t = Variable(args[0].shape) |
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.
can you add so if args[0] is already a variable we just return, see for example exp_canon
|
Nice changes! I think vstack and hstack look neat. One possible refactor to be consistent with the rest of the code: in, for example, the jacobian of add_expr we have a list with keys_that_require_summing. Can we add that to the hessians of vstack and hstack? After this I'm happy with vstack and hstack. Good job! I'm also fine with the product code, even though I haven't gone through it carefully to understand if there's an easier and simpler way. Let's merge it once you have fixed the small comments above. Thanks! |
|
The product code complexity comes from trying to make it impossible for a division by zero |
Description
Nothing huge, just canon, Jacobian, and Hessian_vec support for cp.prod. Also added Jacobian to hstack/vstack (and therefore bmat).