-
Notifications
You must be signed in to change notification settings - Fork 55
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
Improve non-trivial datatypes #1808
Conversation
Hello again! Thank you for this new pull request 🤩. Please begin by requesting your checklist using the command |
/bot run pr_tests |
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.
There seems to be lines in this PR which aren't tested. Please take a look at my comments and add tests which cover the new code.
If this is modified code which cannot be easily tested in this PR please open an issue to request that this code be either removed or tested. Once you have done that please leave a message on the relevant conversation beginning with the line /bot accept
and referencing the issue.
Similarly if the new code cannot be tested for some reason, please leave a comment beginning with the line /bot accept
on the relevant conversation explaining why the code can't be tested.
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 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.
There seems to be lines in this PR which aren't tested. Please take a look at my comments and add tests which cover the new code.
If this is modified code which cannot be easily tested in this PR please open an issue to request that this code be either removed or tested. Once you have done that please leave a message on the relevant conversation beginning with the line /bot accept
and referencing the issue.
Similarly if the new code cannot be tested for some reason, please leave a comment beginning with the line /bot accept
on the relevant conversation explaining why the code can't be tested.
@@ -972,7 +938,7 @@ | |||
elif len(possible_class_types) == 1: | |||
class_type = possible_class_types.pop().switch_basic_type(dtype) | |||
else: | |||
description = f"({arg1} {cls.op} {arg2})" # pylint: disable=no-member | |||
description = f"({arg1!r} {cls.op} {arg2!r})" # pylint: disable=no-member |
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 code isn't tested. Please can you take a look
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.
/bot accept
Error testing
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 |
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.
Move the storage of rank and order from
ast.basic.TypedAstNode
to an internal property ofast.datatypes.PyccelType
objects (the properties are still exposed viaast.basic.TypedAstNode
). This allows mixed ordered objects to be described and therefore created (e.g.list[float[:,:](order=F)]
). A test is added for this case. This change should make #1583 simple to implement.Making the rank and order part of the type fixes #1821 . This provides enough information to allow lists of arrays. Fixes #1721
In order to correctly index types such as
list[float[:,:](order=F)]
the creation of anIndexedElement
in the syntactic stage is modified. Instead of collapsing all the indices into oneIndexedElement
anIndexedElement
is created for eachast.Subscript
. In the semantic stage this is collapsed into oneIndexedElement
for each container.E.g. for the following code:
On the devel branch after the syntactic stage the last line is described as
IndexedElement('a', (0, 1, 2))
. This persists to the codegen stage.On this branch after the syntactic stage the last line is described as
IndexedElement(IndexedElement(IndexedElement('a', (0,)), (1,)), (2,))
. After the semantic stage it becomesIndexedElement(IndexedElement('a', (0,)), (1,2))
. The printers are modified to handle this. The generated code is not changed (as support for lists has not yet been added) so the indices are still used together to index a 3D array.Commit Summary
if __name__ == '__main__'
on old documentation examples.static_rank
andstatic_order
properties frompyccel.ast.basic
. These properties are now contained instatic_type
._rank
and_order
properties fromTypedAstNode
s.PyccelOperator.set_shape_rank
functions toPyccelOperator.set_shape
functions (the rank must now be determined at the same time as the type).PyccelOperator._set_order
functions.CmathPolar
order
argument from theAllocate
constructor (the information is now retrieved from the order of the allocated variable).rank
andorder
topyccel.ast.datatypes.PyccelType
.switch_rank
function topyccel.ast.datatypes.HomogeneousContainerType
.container_rank
property topyccel.ast.datatypes.HomogeneousContainerType
.NumpyNDArrayType
by the rank and the orderNumpyNewArray
fromdtype
toclass_type
(to set the rank/order in the subclass).NumpyArray
to handle mixed rank objects (e.g. list of F-ordered arrays).NumpyUfuncBase._set_shape_rank
toNumpyUfuncBase._get_shape_rank
and return the shape and rank to avoid saving the rank unnecessarily.NumpyUfuncBase._set_order
toNumpyUfuncBase._get_order
and pass the rank and return the order to avoid saving unnecessarily.NumpyNDArrayType.__new__
function which redirects rank 0 arrays to scalar types.NumpyNDArrayType.__add__
to handle rank and ordering.NumpyNDArrayType.swap_order
function to change between C and F ordered equivalent types.__str__
or__repr__
functions toPyccelType
s such that printing them gives a valid type annotation.NumpyInt
object to easily find the NumPy integer which has the same precision as Python integers.rank
andorder
arguments frompyccel.ast.type_annotations.VariableTypeAnnotation
constructor (information now available inclass_type
).order
property frompyccel.ast.type_annotations.VariableTypeAnnotation
(rank
is retained for now).IndexedElement
s.pyccel.ast.utilities
.rank
andorder
arguments frompyccel.ast.variable.Variable
constructor (the information is now retrieved from theclass_type
).is_stack_scalar
.is_ndarray
method._is_slice
attribute toIndexedElement
to indicate if an element or a slice of the base is represented.allows_negative_indexes
property toIndexedElement
._print_IndexedElement
to handle multi-levelIndexedElement
s.abs
call in_print_NumpyNorm
Allocate
(to be improved with Strings #459 ).get_type_description
(this is now handled byPyccelType.__str__
).errors.report
to allow the location of aTypeError
raised during aFunctionCall
to be more easily located.ast.Subscript
into oneIndexedElement
object.