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

Refactor argument handling to enforce positional-only and keyword-only arguments using / and * #834

Open
edoaltamura opened this issue Sep 27, 2024 · 5 comments
Assignees
Labels
good first issue Good for newcomers type: design 📐 Relevant to code architecture type: enhancement ✨ Features or aspects to improve
Milestone

Comments

@edoaltamura
Copy link
Collaborator

What should we add?

Description

To improve readability and usability in the Qiskit codebase, we propose refactoring function signatures to clearly define positional-only, positional-or-keyword, and keyword-only arguments using the / and * symbols. Parameters before the slash will be strictly positional, while those after the asterisk will be keyword-only. This approach clarifies the API and aligns with Python standards, as in Scikit-learn.

We can start with function signatures and see which arguments should be made keyword-only or remain positional. Temporary lint disables may be necessary to prevent errors during the transition, as seen in #833.

Example from Pegasos QSVC

def __init__(
        self,
        quantum_kernel: BaseKernel | None = None,
        C: float = 1.0,
        num_steps: int = 1000,
        precomputed: bool = False,
        seed: int | None = None,
    ) -> None:

into

def __init__(
        self,
        quantum_kernel: BaseKernel | None = None,
        *,
        C: float = 1.0,
        num_steps: int = 1000,
        precomputed: bool = False,
        seed: int | None = None,
    ) -> None:
@edoaltamura edoaltamura added good first issue Good for newcomers type: design 📐 Relevant to code architecture type: enhancement ✨ Features or aspects to improve labels Sep 27, 2024
@edoaltamura edoaltamura added this to the 0.9.0 milestone Sep 27, 2024
@tsmanral
Copy link

Hi @edoaltamura. Does this issue still exists ? If yes, I'd like to assign it to myself and contribute. Please let me know.

@edoaltamura
Copy link
Collaborator Author

Hi @tsmanral, it does! Thanks for your interest in this project. You're very welcome to scan the code for definitions that need an update. I'd like to make two suggestions:

  • Keep a checklist of files/code instances that need the change either in this issue or in a draft pull request - this is useful for logging purposes and would also help us give you a hand if you need it.
  • You can consult the guidelines for contributions in CONTRIBUTING.md. Specifically, the make lint command shortcut will be useful for this fix.

@edoaltamura
Copy link
Collaborator Author

Also related to linting, this old issue #159 might be tackled with a similar approach. Numpy typing has become more stable since the issue was first opened, see https://numpy.org/doc/stable/reference/typing.html

@tsmanral
Copy link

Thank you @edoaltamura. I'll review this first to identify the changes, then I'll create a PR from my fork to the main repository.

@edoaltamura
Copy link
Collaborator Author

The syntax related to this issue might need to be extended to the V2 primitives, now merged in main, see #843.
@tsmanral please ask if there's anything you need help with! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: design 📐 Relevant to code architecture type: enhancement ✨ Features or aspects to improve
Projects
None yet
Development

No branches or pull requests

2 participants