-
Notifications
You must be signed in to change notification settings - Fork 376
[WIP] Added support for LinearEqualityToPenalty converter accepting a list of penalty terms #1322
Conversation
…of penalty terms (#1294) * now penalty can either be a single float (backward compatible) or a list of float that applies to linear constraints correspondingly * updated docstring for the change
* added test for exception when length of penalty list does not match number of constraints
Looks good. Can we have support for |
I think int makes sense. It seems np.int and np.float are equivalent to python built in int and float type so as long as we support int and float the np version will work. If we support int, do also wnat to support mixed int and float in a list? If we want the penalty "list" to be as compatible as possible (i.e. compatible with all iterable objects), maybe we can just construct a list out of it and test the element type. So, with int and float (and mixing) and general iterable object support: if self._penalty is None:
penaltys = self._auto_define_penalty()
elif isinstance(self._penalty, (float, int)):
penaltys = [self._penalty] * num_constraint
else:
try:
penaltys = list(self._penalty)
except TypeError as e:
raise QiskitOptimizationError('Unsupported penalty type: {}'.format(
type(self._penalty)))
if not all(isinstance(penalty, (float, int)) for penalty in penaltys):
raise QiskitOptimizationError('Unsupported penalty type') |
This looks very good now :) Thank you. |
Also, it seems that it is also failing Mypy testing.
What is this testing? How can I test this locally? BTW, I have run all unittest. |
Also, I would appreciate some feedback on unittest. Do my tests make sense? What else do I need to put in unittest? I suppose I should test mixed int float list. Should I also test np.array? np array with mixed int float? Other unsupported type? |
If you have run from qiskit-aqua folder: pip install -r requirements-dev.txt |
* added support for general iterable object with mixed int and float
For the mypy type test, I tried to annoate penalty as we supports more type: def __init__(self, penalty: Union[float, int, Iterable, None] = None) -> None: @property
def penalty(self) -> Union[float, int, Iterable, None]: @penalty.setter
def penalty(self, penalty: Union[float, int, Iterable, None]) -> None But I am still getting many errors like this:
I would appreciate help on proper type hints.
|
@@ -35,6 +35,9 @@ def __init__(self, penalty: Optional[float] = None) -> None: | |||
""" |
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.
Given the additional types that can now be passed as penalty param I think it needs to change from
penalty: Optional[float] = None
to
penalty: Optional[Union[float, List[float]]] = None
where int is implied by virtue of having float there so does not need to be specific.
# If penalty is None, set the penalty coefficient by _auto_define_penalty() | ||
if self._penalty is None: | ||
penalty = self._auto_define_penalty() | ||
penalties = self._auto_define_penalty() | ||
elif isinstance(self._penalty, (float, int)): |
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.
This type of checking here could check instead for list or numpy array and if not then put the quantity in a list. This would then avoid explicit float/int tests. We say a float, or List[float] upfront so it should be clear this is what should be passed - typehints state that int is implied too when you say float.. This way there is no a need to check on the contents. It may fail later when its used of course when the penalty is part of the expression. It seems a lot less explicit checking.
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.
Thanks for the feedback. It seems like there is a trade off between how strict the test is for the outer iterable object and the test for inner elements.
I wanted to be as general as possible for the iterable objects so that other iterable types such as pandas.DataFrame or tuple can also be used. So I have to check the only types that we allow as a single value (i.e. int/float) before construct a list out of that iterable objects.
If on the other hand, we only want to support list and numpy array and want to be as general as possible for the inner element (e.g. Decimal, complex (not sure if that makes sense for penalty)) I think it would be fine to avoid explicit float/int test.
Also, is performance an issue we need to consider? It seems to me that type checking at the beginning is slightly more efficient then doing calculation in the python for loop and waiting for error.
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.
In general you won't find this kind of type checking - especially over list contents. A user would be expected to provide input that conformed to being used as the stated kind, as per typehint/docstring etc. Evidently if it cannot be used in the expression as needed then Python will raise an exception.
If the user has an iterable object its really easy for them to make a list (as you do) before calling the method so given a input that takes a list, or a single instance of what is in the list, is not so onerous. Hence checking for list, and wrapping it if not as a list, is done elsewhere in this repo for example. Having an input of a list/ndarray is typically what is supported throughout - where this meets an end user, in my mind there is no real need to make the code here more complex I think to save the end user a line or two of very simple code to make a list if they have something else.
Performance in this case should not be an issue - I doubt these lists are very big. Either way in the normal case, that everything is correct, I would argue the code as you propose will be slower since it is always checking types before letting it be used.
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.
What if only checked for iterable and wrapped if it is not one?
if not hasattr(penalty, "__len__"):
penalty = [penalty] * num_constraint
elif len(penalty) == 1:
penalty = [penalty[0]] * num_constraint
This also works with a list of single item.
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 am not sure that is a good check for iterable. I thought about typehint of Iterable[float], but I believe there are aspects of typing around this are being deprecated in Python 3.9. Given a user has an iterable x they can simply do list(x) themselves to pass it in, and that seems like not too burdensome on an end-user and keeps the code here simple.
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.
This is very reasonable, but I am also very much like passing iterables interchangeably to functions, as is with most Python libraries, specially lists and numpy arrays. Anyhow, for this code we need to check if the input is a single float/int (or a list of single item) then replicate that for all constraints. This convoluted the checking step.
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.
Although I am not familiar with Qiskit codebase and convention, if I may offer my 2 cents, I would vote for general iterable since that seems to be the case for numpy and pandas. As for not checking for int/float, I think it is okay as long as there won't be problem caused later on (e.g. complex type) that might not throw an exception during calculation.
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.
Please let me know if you guys want me to do just list/np.array or general iterable and whether to check int/float.
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 think doing just list/np.array for checking is ok. Right now, I don't have any other use cases using general iterable.
Given the move/refactor that is about to take place, and given this has been around for some while I am thinking it should be closed. If it turns out its something that is wanted then it can be replicated if needs be. Thoughts/objections? |
Qiskit Optimization has now been moved/refactored into its own repository https://github.com/Qiskit/qiskit-optimization As such, apart from bug fixes that are deemed necessary, the code here is in effect frozen. If this function is still deemed worthwhile, given it has not been worked on for while, then an issue can be raised on the new repo to discuss ahead of any implementation - I'm noting that the details here were still under discussion. |
Summary
Fixes #1294
Added support for LinearEqualityToPenalty converter accepting a list of penalty terms (#1294)
* now penalty can either be a single float (backward compatible) or a list of float that applies to linear constraints correspondingly
* updated docstring for the change
For the tests:
* added peanlty list version for all existing test
* added test for exception when length of penalty list does not match number of constraints
Details and comments
Please let me know if any change is needed! Thanks!