fix cross reference of classes from terra #499
Conversation
Ryand1234
requested review from
stefan-woerner,
manoelmarques,
woodsp-ibm and
adekusar-drl
as code owners
|
|
…and1234/qiskit-machine-learning into update_terra_class_references
|
Hi @adekusar-drl in this issue and PR we were changing references from class that belongs to terra. Will it be okay to update references for classes which belongs to this repo like QuantumKernal in this PR? or should I create new PR for it? |
|
@Ryand1234 certainly, you can update reference to all classes you find in the docstrings, either they are in Terra or here in QML. The point is that if the references are properly formatted, they become hyper links in the online documentation. |
|
For the CLA it goes by the email in the commits which comes from your local git config. If it cannot resolve that email to a github user id then it shows the user name from your git config. Maybe you used different machines to do this PR. One way to fix this is to add the other email to your github account. |
woodsp-ibm
added
documentation 📖
Community PR 🌐
Pull Request Test Coverage Report for Build 3330879471
💛 - Coveralls |
| @@ -84,11 +84,13 @@ def __init__( | |||
|
|
|||
| Raises: | |||
| ValueError: | |||
| - if ``quantum_kernel`` is passed and ``precomputed`` is set to ``True``. To use | |||
| a precomputed kernel, ``quantum_kernel`` has to be of the ``None`` type. | |||
| - if :class:`~qiskit_machine_learning.kernels.FidelityQuantumKernel` is passed and | |||
There was a problem hiding this comment.
I think this change to make things specific makes it incorrect. The quantum_kernel parameter must be None if you say you will use a precomputed kernel when that parameter is True. A quantum_kernel by type just has to be subclass of BaseKernel - so by making some specific reference it changes the meaning somewhat.
Now its true that if precomputed is False and you do not pass a quantum_kernel, i.e. you leave it at None, then it creates an instance of that specific subclass - but overall its not limited to that. Arguably this default when None and precomputed is False ought to be noted in the docstring above.
| TypeError: | ||
| - if ``quantum_kernel`` neither instance of | ||
| :class:`~qiskit_machine_learning.kernels.BaseKernel` nor ``None``. | ||
| - if :class:`~qiskit_machine_learning.kernels.FidelityQuantumKernel` neither instance | ||
| of :class:`~qiskit_machine_learning.kernels.BaseKernel` nor ``None``. |
There was a problem hiding this comment.
This I think can be completely removed - the code did raise a TypeError but that seems to have been removed in a recent change since things are now more general around a BaseKernel rather than what was there before.
On thing I do note is that is that this routine raises a ValueError if C <= 0 i.e. not a positive number, which is not included above.
| ansatz: The (parametrized) circuit to be used as an ansatz for the underlying | ||
| :class:`~qiskit_machine_learning.neural_networks.CircuitQNN`. If ``None`` is given | ||
| then the ``RealAmplitudes`` circuit is used. | ||
| then the :class:`~qiskit.circuit.library.RealAmplitudes` circuit is used. | ||
| loss: A target loss function to be used in training. Default value is ``cross_entropy``. | ||
| optimizer: An instance of an optimizer to be used in training. When ``None`` defaults | ||
| to SLSQP. |
There was a problem hiding this comment.
SLSQP here could be written as :class:`~qiskit.algorithms.optimizers.SLSQP` since this is the actual optimizer class it defaults to. This default is actually done in TrainableModel class where it could be altered the same way,
|
@Ryand1234 will you continue to work on this pull request? |
|
@Ryand1234 Are there any updates here? |
|
Closing the PR as stale. |
Summary
Fixes #474
Details and comments
Replaced plain class names from terra with cross references.