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.

Sign up for GitHub

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

Jump to bottom

add abs function to parameterexpression #7497

Closed
wants to merge 12 commits into from
Closed

add abs function to parameterexpression #7497

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
03f43f8
added abs method to parameterexpression
czachow Jan 7, 2022
eff2f34
fixed wrong call in abs function
czachow Jan 7, 2022
966ee8e
changed definition of abs to __abs__
czachow Jan 10, 2022
fa1bae4
additionally implemented __le__, __ge__, __lt__, __gt__ operators
czachow Jan 10, 2022
93e1d6d
fixed requested features and implemented more tests for __lt__, __gt__
czachow Jan 17, 2022
a30b8db
fixed lint errors in parameterexpression and its tests
czachow Jan 17, 2022
1b98392
fixed formatting error in docstrings
czachow Jan 18, 2022
296e6ae
removed __lt__ and friends, fixed recommendations
czachow Jan 19, 2022
420435e
Merge branch 'main' into pr-parameterexpression_abs
czachow Jan 19, 2022
f72c7fe
added more tests for abs function
czachow Jan 19, 2022
7af0db7
fixed lint
czachow Jan 19, 2022
76e8719
added a releasenote to document change
czachow Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions qiskit/circuit/parameterexpression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,15 @@ def __copy__(self):
def __deepcopy__(self, memo=None):
return self

def __abs__(self):
"""Absolute of a ParameterExpression"""
if HAS_SYMENGINE:
return self._call(symengine.Abs)
else:
from sympy import Abs as _abs

return self._call(_abs)

def __eq__(self, other):
"""Check if this parameter expression is equal to another parameter expression
or a fixed value (only if this is a bound expression).
Expand All @@ -490,6 +499,74 @@ def __eq__(self, other):
return len(self.parameters) == 0 and complex(self._symbol_expr) == other
return False

def __lt__(self, other: ParameterValueType) -> bool:
"""Check if this parameter expression is less than another parameter expression
or a fixed value (only if this is a bound expression).
Args:
other (ParameterExpression or a number):
Parameter expression or numeric constant used for comparison
Raises:
TypeError:
- If comparison to number with unbound parameters.
- If comparison to type of other object unsupported.
Returns:
bool: result of the comparison
"""
if isinstance(other, ParameterExpression):
if HAS_SYMENGINE:
from sympy import sympify

return sympify(self._symbol_expr).__lt__(sympify(other._symbol_expr))
else:
return self._symbol_expr.__lt__(other._symbol_expr)
Copy link
Member

Choose a reason for hiding this comment

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

These don't do what you think they do; they don't always return Boolean values. Also, in general it's better to use the Python operations, and let Python handle the numerics; this way can fail if the left-hand side doesn't evaluate __lt__, but the right hand does evaluate __gt__, whereas if you'd used < in that situation, Python would evaluate it correctly.

For not returning a Boolean, see:

In [1]: import sympy
   ...: x = sympy.Symbol("x")
   ...: x + 1 > x
Out[1]: x + 1 > x

elif isinstance(other, numbers.Number):
if len(self.parameters) == 0:
return float(self._symbol_expr) < other
else:
raise TypeError(
"'<' not supported between instances of {type(self)} "
"with unbound parameters {self.parameters} and {type(other)}."
)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like there will be situations where an expression might not have a single numerical value, but can be bound for certain comparisons. For example, abs(x) >= 0 is knowable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the implementation to #7539, please have a look there.

else:
raise TypeError("'<' not supported between instances of {type(self)} and {type(other)}")
czachow marked this conversation as resolved.
Show resolved Hide resolved

def __gt__(self, other: ParameterValueType) -> bool:
"""Check if this parameter expression is greater than another parameter expression
or a fixed value (only if this is a bound expression).
Args:
other (ParameterExpression or a number):
czachow marked this conversation as resolved.
Show resolved Hide resolved
Parameter expression or numeric constant used for comparison
Raises:
TypeError:
- If comparison to number with unbound parameters.
- If comparison to type of other object unsupported.
Returns:
bool: result of the comparison
"""
if isinstance(other, ParameterExpression):
if HAS_SYMENGINE:
from sympy import sympify

return sympify(self._symbol_expr).__gt__(sympify(other._symbol_expr))
else:
return self._symbol_expr.__gt__(other._symbol_expr)
elif isinstance(other, numbers.Number):
if len(self.parameters) == 0:
return float(self._symbol_expr) > other
else:
raise TypeError(
"'>' not supported between instances of {type(self)} "
"with unbound parameters {self.parameters} and {type(other)}."
)
else:
raise TypeError("'>' not supported between instances of {type(self)} and {type(other)}")

def __ge__(self, other: ParameterValueType) -> bool:
return not self.__lt__(other)

def __le__(self, other: ParameterValueType) -> bool:
return not self.__gt__(other)

def __getstate__(self):
if HAS_SYMENGINE:
from sympy import sympify
Expand Down
23 changes: 23 additions & 0 deletions test/python/circuit/test_parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,30 @@ def test_compare_to_value_when_bound(self):

x = Parameter("x")
bound_expr = x.bind({x: 2.3})

czachow marked this conversation as resolved.
Show resolved Hide resolved
self.assertEqual(bound_expr, 2.3)
self.assertTrue(bound_expr < 3.0)
self.assertTrue(bound_expr > 1.0)
self.assertEqual(abs(bound_expr), 2.3)
Copy link
Member

Choose a reason for hiding this comment

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

If we keep the comparison operators __le__ and __ge__, we'd need to those here too.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be best to separate out the tests of abs into their own function; they're not really related to this.

Also, I think we could do with more stringent tests of it; at the moment, if abs is implemented incorrectly this test can still fail. We need to actually test the mathematical properties of abs, for example that it makes negative numbers positive, and complex numbers positive-real. We should also test that it works for symbolic expressions, such as abs(< -x >).

Copy link
Contributor Author

@czachow czachow Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As suggested, I added more tests to the implementation. What do you mean by testing symbolic expressions, such as abs(< -x >)?


def test_raise_if_compare_not_supported(self):
"""Verify raises if compare to object."""
x = Parameter("x")
y = object
Copy link
Member

Choose a reason for hiding this comment

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

You probably meant to instantiate an object here (y = object()), rather than use the type object. It'll work the same, but the way it's written is slightly confusing.


with self.assertRaisesRegex(TypeError, "not supported"):
x.__lt__(y)
Copy link
Member

Choose a reason for hiding this comment

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

It'd be clearer to write x < y if that's what you mean; it's good to test the exact paths you expect to be used.

with self.assertRaisesRegex(TypeError, "not supported"):
x.__gt__(y)

def test_raise_if_compare_to_value_not_bound(self):
"""Verify raises if compare to value and not bound."""
x = Parameter("x")

with self.assertRaisesRegex(TypeError, "unbound parameters"):
x.__gt__(2.3)
with self.assertRaisesRegex(TypeError, "unbound parameters"):
x.__lt__(2.3)

def test_cast_to_float_when_bound(self):
"""Verify expression can be cast to a float when fully bound."""
Expand Down