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

Automagic dispatch for universal functions #883

Merged
merged 43 commits into from
Feb 22, 2024

Conversation

IvanIsCoding
Copy link
Collaborator

@IvanIsCoding IvanIsCoding commented May 27, 2023

Follow up of #882, albeit more agressive on shortening the boilerplate amount of code for universal functions

This PR introduces a new _rustworkx_dispatch decorator that:

  • Creates the singledispatch for func
  • Automatically registers digraph_func for PyDiGraph
  • Automatically registers graph_func for PyGraph

The decorator is based on the original functools.singledispatch. It should simplify the amount of code we need to write an universal function. It does add a little bit of "magic" with functools and importlib, but overall it should be maintainable

@coveralls
Copy link

coveralls commented May 27, 2023

Pull Request Test Coverage Report for Build 7997454293

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 96.483%

Files with Coverage Reduction New Missed Lines %
src/shortest_path/all_pairs_bellman_ford.rs 6 95.53%
Totals Coverage Status
Change from base Build 7996586980: -0.03%
Covered Lines: 16815
Relevant Lines: 17428

💛 - Coveralls

@IvanIsCoding
Copy link
Collaborator Author

This is ready for review

@IvanIsCoding IvanIsCoding modified the milestones: 0.14.0, 0.15.0 Oct 18, 2023
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I scanned through all this and all the re-registrations look right to me, including the is_isomorphic_node_match rewrite to not be a direct dispatcher itself.

I personally avoid magic name inference like this, but this will fail loudly and immediately at import time if something's not available, so I'd agree it should be maintainable enough (and the line-count reduction is nice for sure).

My ticks don't count on rustworkx, but you can have one in principle (with or without the importlib change).

rustworkx/__init__.py Show resolved Hide resolved
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, we end up doing this a lot so abstract away the boilerplate makes a lot of sense here.

@mtreinish mtreinish merged commit f3cc7fd into Qiskit:main Feb 22, 2024
25 checks passed
@IvanIsCoding IvanIsCoding deleted the magic_dispatch branch March 4, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants