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

Platform backends API and logic structure #17

Merged
merged 39 commits into from
Oct 8, 2024
Merged

Platform backends API and logic structure #17

merged 39 commits into from
Oct 8, 2024

Conversation

Doomsk
Copy link
Member

@Doomsk Doomsk commented Aug 29, 2024

Closes #7

Add platforms API backends and enables use for pyqtorch and pulser/qutip backends and extend it to (user) custom backends.

ping @kaosmicadei @jpmoutinho @Roland-djee since there are limited spots for reviewers)

@Doomsk Doomsk requested a review from kaosmicadei August 29, 2024 08:18
@Doomsk Doomsk self-assigned this Aug 29, 2024
@Doomsk Doomsk removed the request for review from kaosmicadei August 29, 2024 08:18
@Doomsk Doomsk requested a review from kaosmicadei August 29, 2024 08:18
@Doomsk
Copy link
Member Author

Doomsk commented Sep 3, 2024

@Roland-djee you can review it as well (I cannot add you and Kaonan at the same because of github limitations). Actually, let me know if you want to take over on the review process so you can be more familiar with the platforms logic and structure

@Doomsk Doomsk requested review from RolandMacDoland and removed request for kaosmicadei September 5, 2024 07:36
Copy link
Contributor

@RolandMacDoland RolandMacDoland left a comment

Choose a reason for hiding this comment

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

Thank you @Doomsk for the amazing work. It would have been a bit better to split into several iterations as it is somewhat difficult to review properly such a big chunk of code. Some comments from me on the first half. I'll review the second half when you've addressed them. Generally, I would strive not to use too much typing types. You use Literal in several places that doesn't have the same guarantees as StrEnum in terms of robustness.

Copy link
Member

@pimvenderbosch pimvenderbosch left a comment

Choose a reason for hiding this comment

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

I tried to review on the request of Eduardo using the current knowledge I have. I'm not yet enough up to speed to review all the details of some areas of the code.

@RolandMacDoland
Copy link
Contributor

Hey @Doomsk is this reviewable/mergeable ? We need to move forward on this for the public release.

@Doomsk
Copy link
Member Author

Doomsk commented Oct 1, 2024

Hey @Doomsk is this reviewable/mergeable ? We need to move forward on this for the public release.

@Roland-djee you mentioned you would split in two parts the review. Changes were made already accounting for your and Pim's suggestions so far.

@RolandMacDoland
Copy link
Contributor

Hey @Doomsk is this reviewable/mergeable ? We need to move forward on this for the public release.

@Roland-djee you mentioned you would split in two parts the review. Changes were made already accounting for your and Pim's suggestions so far.

Alright, so I'll proceed with the second half of my review and we can move on this.

@RolandMacDoland RolandMacDoland self-requested a review October 1, 2024 10:10
Copy link
Contributor

@RolandMacDoland RolandMacDoland left a comment

Choose a reason for hiding this comment

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

Hey @Doomsk thanks really this is great work. Here are some further comments to this massive PR. Generally, it'd be great to improve on code documentation (doctrings, comments, naming, etc) as the logic is sometimes a bit confusing.

Copy link
Contributor

@RolandMacDoland RolandMacDoland left a comment

Choose a reason for hiding this comment

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

Hiya @Doomsk @kaosmicadei we're getting there. Main points are to improve docstrings and tests and resolve pending discussions. Thanks !

@Doomsk Doomsk merged commit 7910a64 into main Oct 8, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Adapt pql to platforms logic and its backends
4 participants