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
Codegen c multi-dimensional locals #21190
Conversation
✅ Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.8. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
Thank you for your contribution. This will also need a test. |
@gouarin I think you know the |
@bjodah thanks. I will review this PR before the end of this week. |
Hi @rols121, few days ago I added a comment in your test case. |
Hey @gouarin, no, unfortunately not. Where do I see comments? It still says "no reviews" at the top of this PR. |
A = Matrix(A_sym) | ||
b = Matrix(b_sym) | ||
c = A*b | ||
cgen = CCodeGen(project="test", cse=True) |
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.
Why do you use cse here? The example is more complicated to read, no?
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 had to use cse so that the code reached the section where c locals are printed. If I did not use cse, then there where no locals and the "raise CodeGenError("Only column vectors are supported in local variabels. Local result {} has dimensions {}".format(result, dims))" line was not reached.
The test is a bit simple as it does not show cse being used but it does however force an array from a multi-dimensional input.
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.
of course! Thanks.
Sorry, my bad. |
Brief description of what is fixed or changed
Sympy did not allow for codegen with multi-dimensional arrays as arguments that would require a local variable copy. This change just enforces that multi-dimensional arrays are converted to a single dimension array with the equivalent amount of elements. The de-referencing of the new local array already happened correctly.
Other comments
This could be improved upon even more by having a flag for "copy arguments", which, if set has the current behavior, and if not then just uses the argument instead of making a copy and then operates on the argument. It may not be the neatest solution but contiguous multi-dim arrays could just be treated as a single dim array if the user is willing to pass the pointer to the very first element. This change just is just a starting point to allow the use of sympy Matrix types with more that one dimension for c code generation.
Release Notes