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

🚸 Add Basic Python Bindings for NAComputation #751

Closed
wants to merge 2 commits into from
Closed

Conversation

ystade
Copy link
Collaborator

@ystade ystade commented Nov 13, 2024

Description

I need the export functionality of the NAComputation to a string in the Python bindings of MQT QMap. This PR creates a binding for that on the Python side.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@ystade ystade added usability Anything related to usability Core Anything related to the Core library and IR labels Nov 13, 2024
@ystade ystade self-assigned this Nov 13, 2024
@burgholzer burgholzer added the python Anything related to Python code label Nov 13, 2024
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

This is missing a couple of small things:

  • Python stubs similar to https://github.com/cda-tum/mqt-core/tree/main/src/mqt/core/ir (including docstrings)
  • A small test on the Python side in the test suite to make sure this works as expected. (we currently do not track coverage of the bindings code, which is unfortunate, but I haven't found time to figure out how to actually do that)

Side note: I'd still be in favour of trying to unify the kinds of computations across the MQT in the future so that this additional bindings module is not even necessary.

Bigger comment: I am almost certain this will not work right away in QMAP and will rather depend on #662 and cda-tum/mqt-qmap#418, which still require some work to get right.
At the moment, you are probably better off to simply expose the NAComputation class as part of the QMAP bindings.

@@ -85,7 +85,7 @@ ninja.version = ">=1.10"
build-dir = "build/{wheel_tag}/{build_type}"

# Only build the Python bindings target
build.targets = ["ir"]
build.targets = ["ir", "na"]
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add an interface target in the src/python/CMakeLists.txt file after including all the subdirectories that links all the bindings targets together.
That would let us use a single string here.
Maybe its also fine to leave this as is, given how this will change with #662 again anyway.

@burgholzer
Copy link
Member

Closing this for now. Let's revisit once that makes more sense.
In the meantime, QMAP seems to be fine without this.

@burgholzer burgholzer closed this Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Anything related to the Core library and IR python Anything related to Python code usability Anything related to usability
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants