Skip to content
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 np.array implementation #1256

Merged
merged 27 commits into from
Aug 25, 2023
Merged

Improve np.array implementation #1256

merged 27 commits into from
Aug 25, 2023

Conversation

jalalium
Copy link
Member

@jalalium jalalium commented Dec 13, 2022

  • Ensure np.array can be called with one variable argument (previously this caused an infinite loop over the elements of the array).
  • Simplify _print_NumpyArray to reduce duplication and fix Wrong use of transpose in np.array() in fortran #1241 and fix duplicate Copy of a fortran array uses unnecessary transpose #1257.
  • Improve np.array implementation to correct the default order and add the ndmin parameter.
  • Add tests for new code.
  • Clean up test_arrays.py so failures reference the relevant issue, tuples use == instead of np.array_equal, and a more comprehensive check is carried out on returned arrays including checking the datatype and ordering.

@EmilyBourne
Copy link
Member

@jalalium Thanks for your pull request. Please have a quick look at the review process in the developer docs to see how we organise PRs in pyccel and add the appropriate labels. Thanks!

@yguclu
Copy link
Member

yguclu commented Dec 13, 2022

@jalalium Please add a unit test which reproduces issue #1241. This should fail before your changes.

@jalalium
Copy link
Member Author

@jalalium Please add a unit test which reproduces issue #1241. This should fail before your changes.

While trying to add tests for both epyccel and pyccel, I noticed that their behavior are inconsistent. Epyccel performs an extra transposition of the fortran code. Basically, the output of the epyccel function and the output one would get from generating fortran code are inconsistent.
For the following function:

def f():
    import numpy as np
    a = np.array([[1,  2,  4,  8, 16],
             [1,  2,  4,  8, 16],
             [1,  2,  4,  8, 16],
             [1,  2,  4,  8, 16],
             [1,  2,  4,  8, 16]], order="F")
    b = np.array(a, order="F")
    return b

epyccel outputs the following

f2 = epyccel(f, language='fortran')
print(f2())
[[ 1  1  1  1  1]
 [ 2  2  2  2  2]
 [ 4  4  4  4  4]
 [ 8  8  8  8  8]
 [16 16 16 16 16]]

While the code generated in Fortran outputs the expected result.

@EmilyBourne
Copy link
Member

I think there are 2 bugs here. I am opening issues

@EmilyBourne EmilyBourne added blocked Cannot be solved/merged until something else is fixed and removed needs_initial_review labels Dec 14, 2022
@EmilyBourne
Copy link
Member

Unfortunately after looking into this I don't think you can add tests until #1128 and #1260 are merged. I am therefore marking this PR as blocked. Hopefully @ohachim can get back to PR #1128 soon so we can unblock these issues.

@EmilyBourne EmilyBourne marked this pull request as draft December 14, 2022 15:26
@yguclu yguclu changed the title issue1241: Using steps in views with np.array() bug in fortran Avoid unnecessary transposition of Fortran-ordered arrays Dec 16, 2022
@EmilyBourne
Copy link
Member

Blocked by #1258

@aihya aihya mentioned this pull request Jan 11, 2023
@EmilyBourne EmilyBourne removed the blocked Cannot be solved/merged until something else is fixed label Aug 7, 2023
@EmilyBourne EmilyBourne marked this pull request as ready for review August 15, 2023 06:51
@EmilyBourne EmilyBourne marked this pull request as draft August 15, 2023 06:51
@EmilyBourne EmilyBourne reopened this Aug 15, 2023
@EmilyBourne EmilyBourne marked this pull request as ready for review August 15, 2023 06:51
@yguclu
Copy link
Member

yguclu commented Aug 23, 2023

/bot run pr_tests

Copy link

@pyccel-bot pyccel-bot bot left a 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.

@pyccel-bot
Copy link

pyccel-bot bot commented Aug 23, 2023

Hey @pyccel/pyccel-dev ! @jalalium has just created this great new pull request! Check it out and let me know what you think!

@yguclu
Copy link
Member

yguclu commented Aug 23, 2023

/bot run pr_tests

@github-actions github-actions bot marked this pull request as draft August 23, 2023 15:59
pyccel/ast/numpyext.py Outdated Show resolved Hide resolved
@yguclu
Copy link
Member

yguclu commented Aug 23, 2023

/bot run pr_tests

Copy link
Member

@yguclu yguclu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I have a couple of questions.

tests/epyccel/test_arrays.py Show resolved Hide resolved
tests/epyccel/test_arrays.py Show resolved Hide resolved
tests/epyccel/test_arrays.py Show resolved Hide resolved
tests/epyccel/test_arrays.py Show resolved Hide resolved
tests/epyccel/test_epyccel_return_arrays.py Outdated Show resolved Hide resolved
EmilyBourne and others added 2 commits August 23, 2023 20:00
@EmilyBourne
Copy link
Member

/bot run linux

@EmilyBourne EmilyBourne marked this pull request as ready for review August 24, 2023 18:09
Copy link

@pyccel-bot pyccel-bot bot left a 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.

@pyccel-bot
Copy link

pyccel-bot bot commented Aug 24, 2023

@yguclu, @jalalium 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?

@pyccel-bot pyccel-bot bot requested a review from yguclu August 24, 2023 18:34
@pyccel-bot pyccel-bot bot added Ready_to_merge Approved by senior developer. Ready for final approval and merge and removed needs_initial_review labels Aug 25, 2023
@yguclu yguclu merged commit 536be30 into devel Aug 25, 2023
14 checks passed
@yguclu yguclu deleted the issue1241 branch August 25, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug enhancement Language:Fortran Ready_to_merge Approved by senior developer. Ready for final approval and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy of a fortran array uses unnecessary transpose Wrong use of transpose in np.array() in fortran
3 participants