Extend docstrings of loss functions #64

beichensinn · 2021-04-20T14:20:54Z

Summary

Added docstrings.
Closes #49.

…rning into extend_loss_docstring_#49

qiskit_machine_learning/utils/loss_functions/loss_functions.py

woodsp-ibm · 2021-04-20T14:54:34Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

+        This class computes the L1 loss: sum |target - predict|
+    """
+
+    def evaluate(self, predict: np.ndarray, target: np.ndarray):


When overridding a base class, where we have (and should have) documentation on base class method, there is no need to duplicate/repeat it on derived classes. The base class documentation is inherited via Sphinx which means it can be updated/managed in one place rather than having to deal with copies of it all over the place in derived classes. I think we can say in the base class that it Raises: QiskitMachineLearingError if the shape is incorrect or if used for a classification it does not support, e.g it can only handle binary but is given multiclass

woodsp-ibm · 2021-04-20T15:01:37Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

        return self.evaluate(predict, target)

    @abstractmethod
-    def evaluate(self, predict, target):
-        """ evaluate """
+    def evaluate(self, predict: np.ndarray, target: np.ndarray):


I see in the derived classes they document this returning a float and the meaning of the float. In which case I think the signature here should be updated to return a float ie def evaluate(self, predict: np.ndarray, target: np.ndarray) -> float: and document here the meaning of that return.

For both evaluate and gradient we expect them to return a float right so the caller of a concrete sub-class knows what to expect. Hence in the abstract methods here the signature should show the float return typehint and the Returns in the docstring document the meaning of the float. The docstrings here would then be complete meaning no docstring should be needed on the derived classes - which at present all have a copy/paste of the same Returns text when they should all just simply have no docstring i.e inherit the one from the base class.

woodsp-ibm · 2021-04-20T15:01:49Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

        raise NotImplementedError

    @abstractmethod
-    def gradient(self, predict, target):
-        """ gradient """
+    def gradient(self, predict: np.ndarray, target: np.ndarray):


Similar comment on return value.

into extend_loss_docstring_#49 � Conflicts: � qiskit_machine_learning/utils/loss_functions/loss_functions.py

adekusar-drl · 2021-05-12T15:32:48Z

@woodsp-ibm if you could take a look again at the PR.

woodsp-ibm · 2021-05-13T18:55:14Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

        return self.evaluate(predict, target)

    @abstractmethod
-    def evaluate(self, predict, target):
-        """evaluate"""
+    def evaluate(self, predict: np.ndarray, target: np.ndarray):


Suggested change

def evaluate(self, predict: np.ndarray, target: np.ndarray):

def evaluate(self, predict: np.ndarray, target: np.ndarray) -> float:

The typehint should show it returns a float

Also there should be a Returns: entry in the docstring that states the meaning of the return value

woodsp-ibm · 2021-05-13T18:56:19Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

        raise NotImplementedError

    @abstractmethod
-    def gradient(self, predict, target):
-        """gradient"""
+    def gradient(self, predict: np.ndarray, target: np.ndarray):


Suggested change

def gradient(self, predict: np.ndarray, target: np.ndarray):

def gradient(self, predict: np.ndarray, target: np.ndarray) -> float:

The typehint should show the return type

And likewise there should be a Returns: entry in the docstring that states the meaning of the return value

woodsp-ibm · 2021-05-13T19:02:13Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

        raise NotImplementedError

    @staticmethod
-    def _validate(predict, target):
+    def _validate(predict: np.ndarray, target: np.ndarray):


Suggested change

def _validate(predict: np.ndarray, target: np.ndarray):

def _validate(predict: np.ndarray, target: np.ndarray) -> Tuple[np.ndarray, np.ndarray]:

The typehint should have the return type. The docstring has a Returns entry so thats good. Tuple will need importing from typing though for this.

I must admit this method is kinda confusing. If it just needs to validate the shapes why does it need to return them since the caller already has them? Now I see it makes a numpy array of them, but then the typehint says it takes a numpy array and not something like a list where I could see it adjusting the types as needed as well as doing some validation, with the (possibly) adjusted values returned,

woodsp-ibm · 2021-05-13T19:07:54Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

+    L1Loss:
+        This class computes the L1 loss: sum |target - predict|


Duplicating the name of the class into the docstring is not necessary; It should be a simple sentence summary, as you have there already, followed by further paragraph(s) of text describing things for more complicated classes that need it = which I do not think is the case here.

Suggested change

L1Loss:

This class computes the L1 loss: sum |target - predict|

This class computes the L1 loss: sum |target - predict|

woodsp-ibm · 2021-05-13T19:08:25Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

+    L2Loss:
+        This class computes the L2 loss: sum (target - predict)^2


Suggested change

L2Loss:

This class computes the L2 loss: sum (target - predict)^2

This class computes the L2 loss: sum (target - predict)^2

…extend_loss_docstring_#49

stefan-woerner

lgtm

woodsp-ibm · 2021-06-02T21:16:19Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

    """

-    def __call__(self, predict, target):
+    def __call__(self, predict: Union[int, np.ndarray], target: Union[int, np.ndarray]) -> float:


So this is now either an integer or an numpy array. The args below mention just the numpy array not a single value. I see the single value is reformed to a numpy array. Does that mean the numpy arrays are arrays of ints too? I.e the predicted and target values are all ints. Maybe the Args docstring can be improved.

So this is now either an integer or an numpy array. The args below mention just the numpy array not a single value. I see the single value is reformed to a numpy array. Does that mean the numpy arrays are arrays of ints too? I.e the predicted and target values are all ints. Maybe the Args docstring can be improved.

No more ints, just numpy arrays.

woodsp-ibm · 2021-06-02T21:16:52Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

+        Args:
+            predict: a numpy array of predicted values using the model
+            target: a numpy array of the true values
+


It would be good to have a Returns here to state what the float return is

It would be good to have a Returns here to state what the float return is

Added a Returns section.

woodsp-ibm · 2021-06-02T21:17:50Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

+        Args:
+            predict: a numpy array of predicted values using the model
+            target: a numpy array of the true values
+


Again it would be good to have a Returns statement here for the float return

Again it would be good to have a Returns statement here for the float return

Same here, added Returns.

woodsp-ibm · 2021-06-02T21:24:40Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

-    def _validate(predict, target):
+    def _validate(
+        predict: Union[int, np.ndarray], target: Union[int, np.ndarray]
+    ) -> Tuple[np.ndarray, np.ndarray]:


So if I pass in an 'int' and an 'int' as predict and target, I will not get back an Tuple of ints rather a Tuple of arrays? I guess its doing a type cast as well as validate that the other methods rely on. Not sure if it would be helpful to expose the duality in the name say _cast_validate or _to_array_validate - its a private method so maybe its less important.

So if I pass in an 'int' and an 'int' as predict and target, I will not get back an Tuple of ints rather a Tuple of arrays? I guess its doing a type cast as well as validate that the other methods rely on. Not sure if it would be helpful to expose the duality in the name say _cast_validate or _to_array_validate - its a private method so maybe its less important.

Now only numpy arrays, renamed to _validate_shapes. I hope it is good enough now.

woodsp-ibm · 2021-06-02T21:28:30Z

qiskit_machine_learning/utils/loss_functions/loss_functions.py

            )
        return predict, target


 class L1Loss(Loss):
-    """L1Loss"""
+    """
+    L1Loss:


The name of the class will be in the docs from the class itself, it does not need to be put into the docstring as it will not really add any value. All it needs is the description of what this class is about ie. the L1Loss: bit can simply be removed. Same in the other classes in this module.

The name of the class will be in the docs from the class itself, it does not need to be put into the docstring as it will not really add any value. All it needs is the description of what this class is about ie. the L1Loss: bit can simply be removed. Same in the other classes in this module.

Agree, docstrings are updated.

into extend_loss_docstring_#49

into extend_loss_docstring_#49 � Conflicts: � qiskit_machine_learning/utils/loss_functions/loss_functions.py

This PR revises documentation of the loss functions. Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com> Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com> Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>

beichensinn added 2 commits April 19, 2021 22:38

loss function docstring extension

3976d62

Merge branch 'master' of https://github.com/Qiskit/qiskit-machine-lea…

05cb4ab

…rning into extend_loss_docstring_#49

beichensinn requested review from manoelmarques, pbark, stefan-woerner and woodsp-ibm as code owners April 20, 2021 14:20

woodsp-ibm added the type: documentation 📖 Issues related to documentation label Apr 20, 2021

woodsp-ibm reviewed Apr 20, 2021

View reviewed changes

manoelmarques added 2 commits April 21, 2021 15:49

Merge branch 'main' into extend_loss_docstring_#49

543974a

Merge branch 'main' into extend_loss_docstring_#49

f931c6f

manoelmarques requested a review from adekusar-drl as a code owner April 27, 2021 16:27

adekusar-drl and others added 13 commits April 27, 2021 21:03

Merge branch 'main' into extend_loss_docstring_#49

1f1d4b6

Merge branch 'main' into extend_loss_docstring_#49

f9d586d

loss function docstring revision

39e5b81

Merge branch 'main' of https://github.com/Qiskit/qiskit-machine-learning

b1594bd

into extend_loss_docstring_#49 � Conflicts: � qiskit_machine_learning/utils/loss_functions/loss_functions.py

loss function docstring revision

ef6640f

black loss function

acba123

fix check 3.9

e49aa16

1) move raises 2) fix float

e93b9bc

try to add raises multiple places

6bd7abc

Merge branch 'Qiskit:main' into extend_loss_docstring_#49

c42dc1f

fix lint, mypy, formatting

891ad9a

Merge branch 'main' into extend_loss_docstring_#49

7473f31

Merge branch 'main' into extend_loss_docstring_#49

b37fd66

woodsp-ibm reviewed May 13, 2021

View reviewed changes

beichensinn and others added 6 commits May 26, 2021 17:10

black

b223742

fixing int

9e40911

Merge branch 'main' into extend_loss_docstring_#49

f1a086f

parameter and return type added as per Steve's comments

c4da0d3

Merge remote-tracking branch 'origin/extend_loss_docstring_#49' into …

c1fc4d3

…extend_loss_docstring_#49

black

da7d902

adekusar-drl requested a review from woodsp-ibm May 28, 2021 11:38

Merge branch 'main' into extend_loss_docstring_#49

d0d1362

adekusar-drl changed the title ~~Extend loss docstring #49~~ Extend docstrings of loss functions Jun 1, 2021

Merge branch 'main' into extend_loss_docstring_#49

07e2fae

stefan-woerner previously approved these changes Jun 2, 2021

View reviewed changes

woodsp-ibm reviewed Jun 2, 2021

View reviewed changes

beichensinn and others added 4 commits June 3, 2021 14:48

Merge branch 'main' into extend_loss_docstring_#49

3d98473

Merge branch 'main' into extend_loss_docstring_#49

a695cfe

Merge branch 'main' of https://github.com/Qiskit/qiskit-machine-learning

6f2ea8e

into extend_loss_docstring_#49

update docstrings

1253dcd

adekusar-drl dismissed stefan-woerner’s stale review via 1253dcd June 16, 2021 11:26

adekusar-drl and others added 6 commits June 16, 2021 12:34

Merge branch 'main' of https://github.com/Qiskit/qiskit-machine-learning

a3cc6d6

into extend_loss_docstring_#49 � Conflicts: � qiskit_machine_learning/utils/loss_functions/loss_functions.py

fix style/lint/mypy

93bf18a

add blank lines

668ca6d

fix spell

666d370

remove unsupported tests

b36af50

Merge branch 'main' into extend_loss_docstring_#49

a8c26dc

woodsp-ibm approved these changes Jun 16, 2021

View reviewed changes

adekusar-drl merged commit df5d9f8 into qiskit-community:main Jun 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend docstrings of loss functions #64

Extend docstrings of loss functions #64

beichensinn commented Apr 20, 2021 •

edited by adekusar-drl

Loading

woodsp-ibm Apr 20, 2021

beichensinn May 7, 2021

woodsp-ibm Apr 20, 2021

beichensinn May 7, 2021

woodsp-ibm May 7, 2021

woodsp-ibm Apr 20, 2021

beichensinn May 7, 2021

adekusar-drl commented May 12, 2021

woodsp-ibm May 13, 2021 •

edited

Loading

beichensinn May 27, 2021

woodsp-ibm May 13, 2021

beichensinn May 27, 2021

woodsp-ibm May 13, 2021 •

edited

Loading

beichensinn May 27, 2021

woodsp-ibm May 13, 2021

beichensinn May 27, 2021

woodsp-ibm May 13, 2021

beichensinn May 27, 2021

stefan-woerner left a comment

woodsp-ibm Jun 2, 2021

adekusar-drl Jun 16, 2021

woodsp-ibm Jun 2, 2021

adekusar-drl Jun 16, 2021

woodsp-ibm Jun 2, 2021

adekusar-drl Jun 16, 2021

woodsp-ibm Jun 2, 2021

adekusar-drl Jun 16, 2021

woodsp-ibm Jun 2, 2021

adekusar-drl Jun 16, 2021

	def evaluate(self, predict: np.ndarray, target: np.ndarray):
	def evaluate(self, predict: np.ndarray, target: np.ndarray) -> float:

	def gradient(self, predict: np.ndarray, target: np.ndarray):
	def gradient(self, predict: np.ndarray, target: np.ndarray) -> float:

	def _validate(predict: np.ndarray, target: np.ndarray):
	def _validate(predict: np.ndarray, target: np.ndarray) -> Tuple[np.ndarray, np.ndarray]:

		L1Loss:
		This class computes the L1 loss: sum \|target - predict\|

		L2Loss:
		This class computes the L2 loss: sum (target - predict)^2

Extend docstrings of loss functions #64

Extend docstrings of loss functions #64

Conversation

beichensinn commented Apr 20, 2021 • edited by adekusar-drl Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adekusar-drl commented May 12, 2021

woodsp-ibm May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodsp-ibm May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefan-woerner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beichensinn commented Apr 20, 2021 •

edited by adekusar-drl

Loading

woodsp-ibm May 13, 2021 •

edited

Loading

woodsp-ibm May 13, 2021 •

edited

Loading