-
Notifications
You must be signed in to change notification settings - Fork 328
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
Extend docstrings of loss functions #64
Changes from 2 commits
3976d62
05cb4ab
543974a
f931c6f
1f1d4b6
f9d586d
39e5b81
b1594bd
ef6640f
acba123
e49aa16
e93b9bc
6bd7abc
c42dc1f
891ad9a
7473f31
b37fd66
063cb7b
79324d7
b93787f
10acb36
cfde2eb
f51660c
1dc3354
2312963
b223742
9e40911
f1a086f
c4da0d3
c1fc4d3
da7d902
d0d1362
07e2fae
3d98473
a695cfe
6f2ea8e
1253dcd
a3cc6d6
93bf18a
668ca6d
666d370
b36af50
a8c26dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -20,24 +20,57 @@ | |||||||
|
||||||||
class Loss(ABC): | ||||||||
""" | ||||||||
Abstract base class for Loss. | ||||||||
Abstract base class for computing Loss. | ||||||||
""" | ||||||||
|
||||||||
def __call__(self, predict, target): | ||||||||
def __call__(self, predict: np.ndarray, target: np.ndarray): | ||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
Returns: | ||||||||
a float value of the loss function | ||||||||
""" | ||||||||
return self.evaluate(predict, target) | ||||||||
|
||||||||
@abstractmethod | ||||||||
def evaluate(self, predict, target): | ||||||||
""" evaluate """ | ||||||||
def evaluate(self, predict: np.ndarray, target: np.ndarray): | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to have a Returns here to state what the float return is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Added a Returns section. |
||||||||
An abstract method for evaluating the loss function | ||||||||
beichensinn marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
""" | ||||||||
raise NotImplementedError | ||||||||
|
||||||||
@abstractmethod | ||||||||
def gradient(self, predict, target): | ||||||||
""" gradient """ | ||||||||
def gradient(self, predict: np.ndarray, target: np.ndarray): | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment on return value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again it would be good to have a Returns statement here for the float return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Same here, added Returns. |
||||||||
An abstract method for computing the gradient | ||||||||
""" | ||||||||
raise NotImplementedError | ||||||||
|
||||||||
@staticmethod | ||||||||
def _validate(predict, target): | ||||||||
def _validate(predict: np.ndarray, target: np.ndarray): | ||||||||
woodsp-ibm marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
Raise: | ||||||||
QiskitMachineLearningError: shapes of predict and target do not match | ||||||||
|
||||||||
Returns: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
""" | ||||||||
|
||||||||
predict = np.array(predict) | ||||||||
target = np.array(target) | ||||||||
if predict.shape != target.shape: | ||||||||
|
@@ -47,9 +80,24 @@ def _validate(predict, target): | |||||||
|
||||||||
|
||||||||
class L1Loss(Loss): | ||||||||
""" L1Loss """ | ||||||||
""" | ||||||||
L1Loss: | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree, docstrings are updated. |
||||||||
This class computes the L1 loss: sum |target - predict| | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||||||||
""" | ||||||||
|
||||||||
def evaluate(self, predict: np.ndarray, target: np.ndarray): | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
Returns: | ||||||||
a float value of the loss function | ||||||||
|
||||||||
def evaluate(self, predict, target): | ||||||||
Raise: | ||||||||
QiskitMachineLearningError: the shape of predict is incorrect | ||||||||
|
||||||||
""" | ||||||||
predict, target = self._validate(predict, target) | ||||||||
|
||||||||
if len(predict.shape) == 0: | ||||||||
|
@@ -61,16 +109,40 @@ def evaluate(self, predict, target): | |||||||
else: | ||||||||
raise QiskitMachineLearningError(f'Invalid shape {predict.shape}!') | ||||||||
|
||||||||
def gradient(self, predict, target): | ||||||||
def gradient(self, predict: np.ndarray, target: np.ndarray): | ||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
Returns: | ||||||||
a float value of the gradient | ||||||||
""" | ||||||||
|
||||||||
predict, target = self._validate(predict, target) | ||||||||
|
||||||||
return np.sign(predict - target) | ||||||||
|
||||||||
|
||||||||
class L2Loss(Loss): | ||||||||
""" L2Loss """ | ||||||||
""" | ||||||||
L2Loss: | ||||||||
This class computes the L2 loss: sum (target - predict)^2 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||||||||
""" | ||||||||
|
||||||||
def evaluate(self, predict: np.ndarray, target: np.ndarray): | ||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
Returns: | ||||||||
a float value of the loss function | ||||||||
|
||||||||
Raise: | ||||||||
QiskitMachineLearningError: the shape of predict is incorrect | ||||||||
|
||||||||
def evaluate(self, predict, target): | ||||||||
""" | ||||||||
predict, target = self._validate(predict, target) | ||||||||
|
||||||||
if len(predict.shape) <= 1: | ||||||||
|
@@ -80,31 +152,78 @@ def evaluate(self, predict, target): | |||||||
else: | ||||||||
raise QiskitMachineLearningError(f'Invalid shape {predict.shape}!') | ||||||||
|
||||||||
def gradient(self, predict, target): | ||||||||
def gradient(self, predict: np.ndarray, target: np.ndarray): | ||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
Returns: | ||||||||
a float value of the gradient | ||||||||
""" | ||||||||
predict, target = self._validate(predict, target) | ||||||||
|
||||||||
return 2 * (predict - target) | ||||||||
|
||||||||
|
||||||||
class CrossEntropyLoss(Loss): | ||||||||
""" CrossEntropyLoss """ | ||||||||
""" | ||||||||
CrossEntropyLoss: | ||||||||
This class computes the cross entropy loss: -sum target * log(predict) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||||||||
""" | ||||||||
|
||||||||
def evaluate(self, predict: np.ndarray, target: np.ndarray): | ||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
def evaluate(self, predict, target): | ||||||||
Returns: | ||||||||
a float value of the loss function | ||||||||
|
||||||||
""" | ||||||||
predict, target = self._validate(predict, target) | ||||||||
|
||||||||
return -np.sum([target[i] * np.log2(predict[i]) for i in range(len(predict))]) | ||||||||
|
||||||||
def gradient(self, predict, target): | ||||||||
""" Assume softmax is used, and target vector may or may not be one-hot encoding""" | ||||||||
def gradient(self, predict: np.ndarray, target: np.ndarray): | ||||||||
""" | ||||||||
Assume softmax is used, and target vector may or may not be one-hot encoding | ||||||||
|
||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
Returns: | ||||||||
a float value of the gradient | ||||||||
|
||||||||
""" | ||||||||
predict, target = self._validate(predict, target) | ||||||||
|
||||||||
return predict * np.sum(target) - target | ||||||||
|
||||||||
|
||||||||
class CrossEntropySigmoidLoss(Loss): | ||||||||
"""This is used for binary classification""" | ||||||||
""" | ||||||||
CrossEntropySigmoidLoss: | ||||||||
This class computes the cross entropy sigmoid loss. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||||||||
|
||||||||
This is used for binary classification. | ||||||||
""" | ||||||||
|
||||||||
def evaluate(self, predict: np.ndarray, target: np.ndarray): | ||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
def evaluate(self, predict, target): | ||||||||
Returns: | ||||||||
a float value of the loss function | ||||||||
|
||||||||
Raise: | ||||||||
QiskitMachineLearningError: Sigmoid Cross Entropy is used for binary classification! | ||||||||
|
||||||||
""" | ||||||||
predict, target = self._validate(predict, target) | ||||||||
|
||||||||
if len(set(target)) != 2: | ||||||||
|
@@ -114,7 +233,16 @@ def evaluate(self, predict, target): | |||||||
x = CrossEntropyLoss() | ||||||||
return 1. / (1. + np.exp(-x.evaluate(predict, target))) | ||||||||
|
||||||||
def gradient(self, predict, target): | ||||||||
def gradient(self, predict: np.ndarray, target: np.ndarray): | ||||||||
""" | ||||||||
Args: | ||||||||
predict: a numpy array of predicted values using the model | ||||||||
target: a numpy array of the true values | ||||||||
|
||||||||
Returns: | ||||||||
a float value of the gradient | ||||||||
|
||||||||
""" | ||||||||
predict, target = self._validate(predict, target) | ||||||||
|
||||||||
return target * (1. / (1. + np.exp(-predict)) - 1) + (1 - target) * ( | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both
evaluate
andgradient
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.