add abs function to parameterexpression #7497

czachow · 2022-01-07T10:37:30Z

Summary

added an abs function to the class ParameterExpression

Details and comments

coveralls · 2022-01-07T11:03:40Z

Pull Request Test Coverage Report for Build 1722151454

3 of 5 (60.0%) changed or added relevant lines in 1 file are covered.
157 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.0002%) to 83.141%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/parameterexpression.py	3	5	60.0%

Files with Coverage Reduction	New Missed Lines	%
qiskit/assembler/run_config.py	1	95.45%
qiskit/utils/run_circuits.py	1	30.79%
qiskit/test/base.py	5	83.17%
qiskit/compiler/assembler.py	6	96.2%
qiskit/execute_function.py	6	90.63%
qiskit/qobj/qasm_qobj.py	20	83.01%
qiskit/qobj/pulse_qobj.py	39	76.79%
qiskit/utils/quantum_instance.py	79	69.49%

Totals
Change from base Build 1714460551:	-0.0002%
Covered Lines:	51997
Relevant Lines:	62541

💛 - Coveralls

qiskit/circuit/parameterexpression.py

peendebak · 2022-01-16T19:00:38Z

qiskit/circuit/parameterexpression.py

+            else:
+                return self._symbol_expr.__lt__(other._symbol_expr)
+        elif isinstance(other, numbers.Number):
+            return len(self.parameters) == 0 and float(self._symbol_expr) < other


If len(self.parameters) !=0 this returns False, but it should raise an exception.

I think the __eq__ method should also have this behaviour. What do you think?

test/python/circuit/test_parameters.py

qiskit/circuit/parameterexpression.py

jakelishman

Thanks for looking at this! The addition of abs looks fine to me, though it needs a release note to go with it.

I'm not sure it's a good idea to add the comparison operators, though. The class is called ParameterExpression, but in general between two instances of it, the comparison is unknowable. Storing the result of < or so turns it into a "relation" not an "expression". For example, see one of the comments below about how sympy doesn't usually return Booleans in comparisons. At the very least, I think adding the comparison operators should be a separate PR, but honestly, I think it would be better to leave them undefined; I don't see any use-cases for them working only very occasionally.

jakelishman · 2022-01-18T16:29:50Z

qiskit/circuit/parameterexpression.py

+            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)}."
+                )


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.

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

qiskit/circuit/parameterexpression.py

jakelishman · 2022-01-18T17:09:43Z

test/python/circuit/test_parameters.py

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


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.

jakelishman · 2022-01-18T17:11:27Z

test/python/circuit/test_parameters.py

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


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.

jakelishman · 2022-01-18T17:14:34Z

test/python/circuit/test_parameters.py

+        self.assertTrue(bound_expr < 3.0)
+        self.assertTrue(bound_expr > 1.0)
+        self.assertEqual(abs(bound_expr), 2.3)


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

jakelishman · 2022-01-18T17:20:21Z

qiskit/circuit/parameterexpression.py

+                return sympify(self._symbol_expr).__lt__(sympify(other._symbol_expr))
+            else:
+                return self._symbol_expr.__lt__(other._symbol_expr)


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

jakelishman

This is better as the simpler PR - it will be safe to add this functionality without issue. Please could you add some more rigorous tests, and also add a release note (see the contributing guide).

jakelishman · 2022-01-19T15:22:35Z

test/python/circuit/test_parameters.py

        self.assertEqual(bound_expr, 2.3)
+        self.assertEqual(abs(bound_expr), 2.3)


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 >).

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

king-p3nguin · 2022-12-20T09:35:33Z

Dear @czachow ,
I want to use the method you created for ParameterExpression. Since your comment on Jan 20th, have you updated the program? If not, I would like to continue your work and make a new pull request.

czachow · 2022-12-20T10:01:32Z

Dear @king-p3nguin, thanks for working on this old PR that would have been forgotten otherwise. In my memory most of the implementation is done. You only need to add more tests like @jakelishman suggested. Feel free to do so!

king-p3nguin · 2022-12-20T15:34:21Z

Thank you very much!

Cryoris · 2022-12-21T09:44:56Z

Closing this as duplicate of #9309.

added abs method to parameterexpression

03f43f8

czachow requested a review from a team as a code owner January 7, 2022 10:37

czachow added 3 commits January 7, 2022 13:42

fixed wrong call in abs function

eff2f34

changed definition of abs to __abs__

966ee8e

additionally implemented __le__, __ge__, __lt__, __gt__ operators

fa1bae4

peendebak suggested changes Jan 16, 2022

View reviewed changes

czachow added 3 commits January 17, 2022 10:07

fixed requested features and implemented more tests for __lt__, __gt__

93e1d6d

fixed lint errors in parameterexpression and its tests

a30b8db

fixed formatting error in docstrings

1b98392

jakelishman reviewed Jan 18, 2022

View reviewed changes

czachow added 2 commits January 19, 2022 10:54

removed __lt__ and friends, fixed recommendations

296e6ae

Merge branch 'main' into pr-parameterexpression_abs

420435e

czachow mentioned this pull request Jan 19, 2022

add __lt__ and friends to parameterexpression #7539

Open

jakelishman reviewed Jan 19, 2022

View reviewed changes

czachow added 3 commits January 19, 2022 18:15

added more tests for abs function

f72c7fe

fixed lint

7af0db7

added a releasenote to document change

76e8719

jakelishman mentioned this pull request Jan 20, 2022

add relational, min, max, floor, ceil, sqrt, arctan2 to ParameterExpression #5278

Closed

HuangJunye added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 21, 2022

king-p3nguin mentioned this pull request Dec 20, 2022

add abs method to ParameterExpression #9309

Merged

Cryoris closed this Dec 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add abs function to parameterexpression #7497

add abs function to parameterexpression #7497

czachow commented Jan 7, 2022

coveralls commented Jan 7, 2022 •

edited

Loading

peendebak Jan 16, 2022

czachow Jan 17, 2022

jakelishman left a comment •

edited

Loading

jakelishman Jan 18, 2022

czachow Jan 19, 2022

jakelishman Jan 18, 2022

jakelishman Jan 18, 2022

jakelishman Jan 18, 2022

jakelishman Jan 18, 2022

jakelishman left a comment

jakelishman Jan 19, 2022

czachow Jan 19, 2022 •

edited

Loading

king-p3nguin commented Dec 20, 2022

czachow commented Dec 20, 2022

king-p3nguin commented Dec 20, 2022

Cryoris commented Dec 21, 2022

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

		self.assertEqual(bound_expr, 2.3)
		self.assertEqual(abs(bound_expr), 2.3)

add abs function to parameterexpression #7497

add abs function to parameterexpression #7497

Conversation

czachow commented Jan 7, 2022

Summary

Details and comments

coveralls commented Jan 7, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1722151454

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman left a comment • 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

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czachow Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

king-p3nguin commented Dec 20, 2022

czachow commented Dec 20, 2022

king-p3nguin commented Dec 20, 2022

Cryoris commented Dec 21, 2022

coveralls commented Jan 7, 2022 •

edited

Loading

jakelishman left a comment •

edited

Loading

czachow Jan 19, 2022 •

edited

Loading