-
Notifications
You must be signed in to change notification settings - Fork 56
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
Clean PythonCodePrinter and add dtype property #1260
Conversation
20df8d3
to
4977c72
Compare
4977c72
to
d2ec6a8
Compare
Hello again! Thank you for this new pull request 🤩. Here is your checklist. Please tick items off when you have completed them or determined that they are not necessary for this pull request:
|
Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with |
/bot run linux |
/bot run docs |
/bot run pr_tests |
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.
Good job ! Your PR is using all the code it added/changed.
Hey @yguclu, @EmilyBourne, this PR is looking pretty good. @EmilyBourne and @sboof911 think it is ready to merge. Could you add your expertise to confirm that this follows all the coding conventions and fits in Pyccel's future plans? Thanks 😄 |
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.
This looks good to me! I have a few comments and questions.
@EmilyBourne, @yguclu has a few questions/comments about your code. Can you go through and see if you agree with them. If not go ahead and explain why. Once you've adressed all the comments let me know with |
Co-authored-by: Yaman Güçlü <yaman.guclu@gmail.com>
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.
Good job ! Your PR is using all the code it added/changed.
@yguclu, @EmilyBourne has been working hard and thinks that they have now replied to or fixed all your comments. Could you take another look at the PR and see if you can approve now? |
Remove the duplicate
_print_NumpyArray
function from PythonCodePrinter. The original_print_NumpyArray
function is more accurate as it specifies thedtype
etc explicitly.Specifying the
dtype
explicitly makes issue #1377 reproducible. To fix this problem support is added fora.dtype
expressions (Fixes #1421) using the similar functionnp.result_type
. The initialisation dtype is saved and used in the Python printer. This is a work-around until #1334 is resolved well enough to define a clear issue. Additionally a warning is raised if the Python code is not consistent. Fixes #1377Additional commits
numpy.bool
in code as it has been deprecated by NumPy. Instead use the default bool (with precision-1
).