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

API Refactor: Enforcing Positional-Only and Keyword-Only Parameters (Issue #834) #901

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

uuu4
Copy link

@uuu4 uuu4 commented Mar 15, 2025

Summary

This pull request addresses Issue #834 by refactoring function signatures throughout the Qiskit Machine Learning codebase. The goal is to improve code clarity, prevent common argument-passing errors, and align the API with modern Python best practices. Key changes include:

  • Improve Clarity:
    Clearly separate required arguments from optional ones, reducing potential argument ordering mistakes.

  • Enhance Consistency:
    Apply a uniform API design across modules to minimize errors and improve maintainability.

Modified Files

The following files have been updated as part of this refactor:

Algorithms

  • qiskit_machine_learning/algorithms/classifiers/neural_network_classifier.py
  • qiskit_machine_learning/algorithms/classifiers/pegasos_qsvc.py
  • qiskit_machine_learning/algorithms/classifiers/vqc.py
  • qiskit_machine_learning/algorithms/objective_functions.py
  • qiskit_machine_learning/algorithms/trainable_model.py

Gradients

  • qiskit_machine_learning/gradients/base/base_estimator_gradient.py
  • qiskit_machine_learning/gradients/base/base_sampler_gradient.py
  • qiskit_machine_learning/gradients/lin_comb/lin_comb_estimator_gradient.py
  • qiskit_machine_learning/gradients/lin_comb/lin_comb_sampler_gradient.py
  • qiskit_machine_learning/gradients/param_shift/param_shift_sampler_gradient.py
  • qiskit_machine_learning/gradients/spsa/spsa_sampler_gradient.py

Neural Networks

  • qiskit_machine_learning/neural_networks/effective_dimension.py
  • qiskit_machine_learning/neural_networks/estimator_qnn.py
  • qiskit_machine_learning/neural_networks/neural_network.py
  • qiskit_machine_learning/neural_networks/sampler_qnn.py

Optimizers

  • qiskit_machine_learning/optimizers/adam_amsgrad.py
  • qiskit_machine_learning/optimizers/aqgd.py
  • qiskit_machine_learning/optimizers/cg.py
  • qiskit_machine_learning/optimizers/cobyla.py
  • qiskit_machine_learning/optimizers/gradient_descent.py
  • qiskit_machine_learning/optimizers/gsls.py
  • qiskit_machine_learning/optimizers/l_bfgs_b.py
  • qiskit_machine_learning/optimizers/nelder_mead.py
  • qiskit_machine_learning/optimizers/nft.py
  • qiskit_machine_learning/optimizers/nlopts/nloptimizer.py
  • qiskit_machine_learning/optimizers/p_bfgs.py
  • qiskit_machine_learning/optimizers/powell.py
  • qiskit_machine_learning/optimizers/qnspsa.py
  • qiskit_machine_learning/optimizers/scipy_optimizer.py
  • qiskit_machine_learning/optimizers/slsqp.py
  • qiskit_machine_learning/optimizers/spsa.py
  • qiskit_machine_learning/optimizers/steppable_optimizer.py
  • qiskit_machine_learning/optimizers/tnc.py
  • qiskit_machine_learning/optimizers/umda.py

State Fidelities & Others

  • qiskit_machine_learning/state_fidelities/base_state_fidelity.py
  • qiskit_machine_learning/state_fidelities/compute_uncompute.py
  • qiskit_machine_learning/variational_algorithm.py

Impact

  • Clear API Usage:
    • Required parameters are enforced as positional-only.
    • Optional parameters are now keyword-only, making function calls self-documenting.

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@OkuyanBoga
Copy link
Collaborator

Hi @uuu4, thank you for your PR submission. Could you please check that all unit tests are passing and ensure your changes follow the contributing guidelines? Also, if possible, group minor changes into a single commit to help reduce overhead.

Copy link
Collaborator

@edoaltamura edoaltamura left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Make sure you don't turn args into kwargs or vice-versa. It's important they stay unchanged otherwise users would experience a lot of breaking changes and their API would stop working. With this in mind, only add the */ notation where needed without changing the order or parsing type of args and kwargs.

For this, PyLint will be a very useful tool, as it detects where */ have been misplaced even before running the unit tests. You should aim at a score of 10/10, and any errors should appear in the terminal. You can find out more about this topic on our contributing guide, together with auto-formatting with black and other good-to-knows.

@edoaltamura edoaltamura marked this pull request as draft March 22, 2025 10:18
@edoaltamura edoaltamura added type: enhancement ✨ Features or aspects to improve on hold 🛑 Can not fix yet labels Mar 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold 🛑 Can not fix yet type: enhancement ✨ Features or aspects to improve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants