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

C to Python wrapper rewrite #1477

Merged
merged 98 commits into from
Aug 22, 2023
Merged

C to Python wrapper rewrite #1477

merged 98 commits into from
Aug 22, 2023

Conversation

EmilyBourne
Copy link
Member

@EmilyBourne EmilyBourne commented Aug 3, 2023

The C-Python API wrapper is unnecessarily complicated. It is very hard to understand and this makes it extremely difficult to develop any new features (e.g. class wrapping, see phase 3 of #453).

This PR completely rewrites the wrapper following the example already set by the bind-C wrapper. An additional stage is added to the treatment which wraps the code. In turn the CWrapperPrinter is stripped back to only retain printing functionalites.

List of changes

  • Create new c_to_python_wrapper module, which contains the class CToPythonWrapper
  • Use CToPythonWrapper object in function create_shared_library of module python_wrapper
  • Provide optional parameters to the clone method of FunctionDef
  • Add new classes BindCArrayVariable and BindCVariable to AST of bind-C (i.e. Fortran-to-C) wrapper
  • Add new classes PyFunctionDef, PyInterface and PyModule to AST of C-to-Python wrapper
  • Fix bug in C functions free_array and free_pointer of Numpy extension
  • Update Pyccel version to 1.9.0

@EmilyBourne
Copy link
Member Author

/bot run linux

@pyccel-bot
Copy link

pyccel-bot bot commented Aug 3, 2023

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:

  • Write a clear PR description
  • Add tests to check your code works as expected
  • Update documentation if necessary
  • Update Changelog
  • Ensure any relevant issues are linked
  • Ensure new tests are passing

@EmilyBourne
Copy link
Member Author

/bot run linux

@EmilyBourne
Copy link
Member Author

/bot run linux

@EmilyBourne
Copy link
Member Author

/bot run linux

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.

Good job! This is a very neat refactoring! I have minor comments as usual.

developer_docs/wrapper_stage.md Outdated Show resolved Hide resolved
pyccel/stdlib/ndarrays/ndarrays.h Show resolved Hide resolved
pyccel/ast/cwrapper.py Outdated Show resolved Hide resolved
tests/pyccel/test_pyccel.py Outdated Show resolved Hide resolved
tests/pyccel/test_pyccel.py Outdated Show resolved Hide resolved
tests/pyccel/test_pyccel.py Outdated Show resolved Hide resolved
tests/pyccel/test_pyccel.py Outdated Show resolved Hide resolved
tests/pyccel/test_pyccel.py Outdated Show resolved Hide resolved
pyccel/codegen/wrapper/c_to_python_wrapper.py Outdated Show resolved Hide resolved
pyccel/codegen/wrapper/c_to_python_wrapper.py Outdated Show resolved Hide resolved
@github-actions github-actions bot marked this pull request as draft August 21, 2023 17:44
@pyccel-bot pyccel-bot bot removed the Ready_for_review Received at least one approval. Requires review from senior developer label Aug 21, 2023
@pyccel-bot
Copy link

pyccel-bot bot commented Aug 21, 2023

@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 /bot mark as ready and we will see if we can get approval.

@EniddeallA
Copy link
Contributor

/bot run linux docs

@EmilyBourne EmilyBourne marked this pull request as ready for review August 22, 2023 07:41
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 pyccel-bot bot added the Ready_for_review Received at least one approval. Requires review from senior developer label Aug 22, 2023
@pyccel-bot
Copy link

pyccel-bot bot commented Aug 22, 2023

@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?

@pyccel-bot pyccel-bot bot requested a review from yguclu August 22, 2023 08:07
@pyccel-bot pyccel-bot bot added Ready_to_merge Approved by senior developer. Ready for final approval and merge and removed Ready_for_review Received at least one approval. Requires review from senior developer labels Aug 22, 2023
@yguclu yguclu merged commit 18d9e2e into devel Aug 22, 2023
13 checks passed
@yguclu yguclu deleted the ebourne_wrapper_rewrite branch August 22, 2023 15:00
@yguclu
Copy link
Member

yguclu commented Aug 23, 2023

@EmilyBourne We forgot to update the version in the CHANGELOG file. I will fix this in the next available PR.

@EmilyBourne
Copy link
Member Author

@EmilyBourne We forgot to update the version in the CHANGELOG file. I will fix this in the next available PR.

Oops. Probably all the changelogs need updating so the changes appear in the correct version

@yguclu
Copy link
Member

yguclu commented Aug 23, 2023

@EmilyBourne We forgot to update the version in the CHANGELOG file. I will fix this in the next available PR.

Oops. Probably all the changelogs need updating so the changes appear in the correct version

We will definitely have some conflicts. Anyway, does this CHANGELOG look good to you?

@EmilyBourne
Copy link
Member Author

@EmilyBourne We forgot to update the version in the CHANGELOG file. I will fix this in the next available PR.

Oops. Probably all the changelogs need updating so the changes appear in the correct version

We will definitely have some conflicts. Anyway, does this CHANGELOG look good to you?

Looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement internals Pyccel's internal behavior, does not affect user experience 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.

Can't use an interface which only differs in rank/order
5 participants