#10360 New sign feature in parameterexpression.py #10571

SameerD-phys · 2023-08-04T16:50:07Z

Added a new feature in parameterexpression.py to display the sign of an expression

…the expression inputted

Summary

I've added a feature that allows a user to display the sign of an inputted expression using an object of the Parameter class.

Details and comments

qiskit-bot · 2023-08-04T16:50:11Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core

CLAassistant · 2023-08-04T16:50:13Z

All committers have signed the CLA.

wshanks · 2023-08-04T16:53:32Z

Closes #10360

wshanks

Besides in-line comments, please make sure you follow the bot's comment above about the CLA and add a release note as described here.

It would also be good to have a test in test_parameters.py that uses sign, though looking now I don't see a test that uses exp or log there either.

qiskit/circuit/parameterexpression.py

wshanks · 2023-08-04T16:59:32Z

qiskit/circuit/parameterexpression.py

+        from sympy import sign
+        val = self._symbol_expr
+        sign_value = sign(val)
+        return sign_value


In qiskit-terra, we try to use symengine (for performance) and fall back to sympy. You can deal with this the way other functions do by referencing _optionals.HAS_SYMENGINE. Also, you want to use self._call(func) rather than just func(val) because the operation should return another ParameterExpression.

So I think it should be something like this, though it would be good to check (with and without symengine):

Suggested change

from sympy import sign

val = self._symbol_expr

sign_value = sign(val)

return sign_value

if _optionals.HAS_SYMENGINE:

import symengine

return self._call(symengine.sign)

else:

from sympy import sign as _sign

return self._call(_sign)

So I did not do self._call(func) on purpose because the issue requires the return type to be numeric. Other methods like log would return a ParameterExpression but I need a numeric.

Why do you need a numeric return type? If the parameter expression is a symbol, don't you need the sign of it to also be a parameter expression until the symbol is assigned a numeric value?

SameerD-phys · 2023-08-04T19:08:53Z

I don't see a file test_parameters.py. I want to show it because I want to explain how a user needs to pass an argument during instantiation for this to work.

wshanks · 2023-08-04T20:49:54Z

I don't see a file test_parameters.py. I want to show it because I want to explain how a user needs to pass an argument during instantiation for this to work.

The file is here.

SameerD-phys · 2023-08-18T17:58:39Z

Hi, I made a few changes and created a PR again. I forgot how to do it here directly but I think PR initiated it somewhere else. Here is the link to it:

#10669

I've signed the CLA. I'm not sure what the second part about adding my email address to this commit to my account is though.

Raghav-Bell · 2023-08-22T05:18:34Z

linting tests are failing. Please check tests using tox locally. For more please check Contribution guide

mergify · 2023-08-23T20:53:35Z

⚠️ The sha of the head commit of this PR conflicts with #10669. Mergify cannot evaluate rules on this PR. ⚠️

wshanks

This is looking close to ready. Thanks for your work, @SamD-1998!

test/python/circuit/test_parameters.py

releasenotes/notes/added-sign-feature-parameterexpression-57693dd69103dc8c.yaml

wshanks · 2023-08-23T21:29:17Z

Also, like Raghav noted, the tests are failing because you need to run black on the code. There is some information in the guide Raghave linked to. Let us know if you need more help with that part.

SameerD-phys · 2023-08-23T21:46:04Z

Hey, I ran black and I think the formatting issues are now fixed. Just a couple questions:

I pushed my commits to my forked repo. I don't want to duplicate a PR again. What should I do?
I've signed the CLA but I'm not sure what the second part about adding my email address to this commit to my account is though. Could you please explain?

wshanks · 2023-08-23T22:20:38Z

The ideal workflow would be that you 1. checkout the negative_feature_10360 branch from your fork, 2. run black, 3. commit the changes from black to your local negative_feature_10360 branch, and 4. push your local negative_feature_10360 back to your fork. Maybe you have done the first three steps here and just need to push back to the right branch on your fork?
When you create a commit with git, it writes your name and email address into the commit. For your commits made locally, the email address shows up as sameerdambal@pop-os.localdomain (probably the default for Pop!OS based on your username). For the commits from you merging in my suggestions, there is a gmail address (I won't copy it here just to avoid inviting spam). I think that gmail address is what is in your GitHub profile and is what is shared with the CLA assistant when you sign it. So the CLA assistant is still unhappy about the pop-os address. You might be able to add that (not real) email address to your GitHub profile to satisfy the CLA assistant. The other option is to use git config user.email <your email address> to set your correct email address it git. Then you could do git rebase -i origin/main to rebase your changes and mark each commit with e to edit it and for each commit then run git commit --amend --reset-author and git rebase --continue to update the author information. Another option would be to squash all the commits down to one since we will squash them when merging the PR any way. Let me know if you need more help with this part. I think there are several ways to update the author information in git and a lot of articles will come up if you search for it. It's a matter of picking which method seems easiest to follow for you.

qiskit/circuit/parameterexpression.py

test/python/circuit/test_parameters.py

wshanks

I think this is pretty ready to merge! We just need to clean up the release note a bit and deal the CLA issue as discussed previously.

I put some suggestions in for the release note. Mostly these are pointing out syntax issues. The release notes get rendered with Sphinx, so they should follow Sphinx syntax. See here for a Sphinx reference.

It is a little tricky to test if the release notes syntax is correct. You need to run sphinx to build the docs to look at. In the Azure Pipelines docs run here, there is an artifact that you can download with the docs from the CI run as well, but currently the release notes are missing (until #10656 is merged).

releasenotes/notes/added-sign-feature-parameterexpression-57693dd69103dc8c.yaml

wshanks · 2023-09-01T15:12:27Z

releasenotes/notes/added-sign-feature-parameterexpression-57693dd69103dc8c.yaml

+    from qiskit.circuit import Parameter
+
+    b = Parameter("phi")
+    sign_value = b.sign()
+    print("sign of Parameter is: ", sign_value)


Suggested change

from qiskit.circuit import Parameter

b = Parameter("phi")

sign_value = b.sign()

print("sign of Parameter is: ", sign_value)

from qiskit.circuit import Parameter

b = Parameter("phi")

sign_value = b.sign()

print("sign of Parameter is: ", sign_value)

I wonder if you want to say something like:

print("Sign of an unassigned Parameter is:", sign_value) print("Sign of a Parameter assigned to -3 is:", sign_value.assign(b, -3))

wshanks · 2023-09-01T15:14:07Z

releasenotes/notes/added-sign-feature-parameterexpression-57693dd69103dc8c.yaml

+---
+features:
+  - |
+    Introduced a new feature that adds support for calculating the sign of the Parameter of the value of an expression of a Parameter class.


Suggested change

Introduced a new feature that adds support for calculating the sign of the Parameter of the value of an expression of a Parameter class.

Added support for expressing the sign of a :class:`ParameterExpression`.

wshanks · 2023-09-01T15:15:13Z

releasenotes/notes/added-sign-feature-parameterexpression-57693dd69103dc8c.yaml

Suggested change

Instead of using numpy or other libraries, the user can use the instance of the ParameterExpression class to calculate the sign.

Instead of assigning a concrete value and using ``numpy.sign`` or other library functions, the user can use the instance of the ParameterExpression class to calculate the sign and can work with the sign before the expression is fully assigned.

SameerD-phys · 2023-09-01T20:27:40Z

Reviewed all recommended changes and pushed a new commit while changing the author information to resolve the CLA issue. I hope it all works out now. Thank you for walking me through all of this!

wshanks

This PR looks almost ready. Something funny might have happened with your editor. Some of the ` that I suggested were ' in your changes. The ` is a special character used by Sphinx. I put some inline suggestions to switch them to what Sphinx needs.

Your name and email looked good on your most recent commit, but that does not rewrite the history for the older commits GitHub is complaining about. We still need to squash down / rewrite the old commits to update the author information. I can do this at the end if you want, but I think part of the goal with this PR is to help your learn the processes around submitting PR's, so I am happy to keep helping you do this on your side.

releasenotes/notes/added-sign-feature-parameterexpression-57693dd69103dc8c.yaml

wshanks · 2023-09-08T15:59:28Z

Sorry, @SamD-1998, what you did here is not a viable way to fix the CLA issue. Yet, somehow the CLA check has a green checkmark which I don't understand. Did you add the pop-os.localdomain email address to your GitHub account?

I am not sure precisely what you did, but I think you rebased with changes from the main branch merged in which cause those commits to be rewritten and now appear as new commits. This makes it look now like this PR is changing many extra files and has extra authors.

I recommend we roll back and try again. I think the commit before was 981c470, so the next steps would be:

git checkout negative_feature_10360
git reset --hard 981c4707347ae2269241485f06dc3abbc5fb5f47
git push -f origin negative_feature_10360

if you are calling this branch negative_feature_10360 and are calling your fork origin in your local git repository.

Sorry about the confusion. I forgot how messy a rebase was after a large branch has been merged in. I think the simplest solution might be to follow the steps given here. In our case, after doing my previous suggestion, this would be something like

git checkout negative_feature_10360
git branch -m negfeat-bak
git checkout main
git pull upstream main
git checkout -b negative_feature_10360
git merge --squash negfeat-bak
git commit
git push -f origin negative_feature_10360

Let me know if that works for you. If will squash all the previous work down to one commit which you can write a commit message for. When the pull request is approved and merged, it will be squashed to a single commit any way.

SameerD-phys · 2023-09-08T19:11:56Z

Oh that looks essentially like rebasing but manually creating a new branch and merging with the older branch. That's clever.

Funnily enough, I'm not sure if I added the pop-os.localdomain to my GitHub at this point since I read a lot of things, tried a lot of things and don't remember all the history. I hope this resolves all the issues.

wshanks · 2023-09-08T19:19:48Z

The current diff looks good to me. We have your changes with no extra changes. Some of my last suggestions for the release notes formatting have not been applied, but they still show up in GitHub as in-line comments, so please look at those.

Also, I still see the pop-os.localdomain email in the first commit c1cd914. It looks like that is a left over initial commit that didn't get squashed. All the changes we want are squashed together in the most recent commit. Maybe you can try the merge --squash procedure one more time to get rid of that commit?

SameerD-phys · 2023-09-08T20:39:37Z

Can I ask for some help?

The first commit c1cd914 was unaffected because I missed changing its pick to squash in the interactive rebase. But as soon as I change it and close the editor, I'm inundated with merge conflicts. I also find myself doing git rebase --skip numerous times to ignore these.

My plan is this:
change pick to squash
git add .
Make formatting changes in releasenotes and then

git checkout negative_feature_10360
git commit
git branch -m negfeat-bak
git checkout main
git pull upstream main
git checkout -b negative_feature_10360
git merge --squash negfeat-bak
git commit
git push -f origin negative_feature_10360

This order because I had accommodated your formatting suggestions multiple times but the git pull upstream main might have undone it.

Could you please tell me how to resolve this?

wshanks · 2023-09-08T20:57:43Z

I would leave the first commit as pick or change it to reword and squash the other commits into it. With reword or with git commit --amend afterwards, you can change the commit message if you want.

Did all the commands run without error for the git merge --squash steps I had suggested? That should have left you with a single commit as well. It required that you had defined upstream as a remote pointing at git@github.com:Qiskit/qiskit.git.

wshanks · 2023-09-09T15:18:04Z

Hmm, you pushed back to an older version without the commits squashed (but still the same final content).

Another option you could try (requires upstream pointing at git@github.com:Qiskit/qiskit.git) is:

git checkout negative_feature_10360
git fetch upstream
git diff --patch upstream/main... > 10360.patch
git reset --hard upstream/main
git apply 10360.patch
git add releasenotes/notes/added-sign-feature-parameterexpression-57693dd69103dc8c.yaml
git commit -a

This avoids rebasing and trying rewrite history and just writes your changes to a patch file, resets your branch to upstream/main, and then dumps you changes back out for you to commit as a new clean commit.

SameerD-phys · 2023-09-11T20:09:08Z

Yes, all commands ran without errors for git merge --squash and I think it did leave with a single commit when I checked in git log. (Well there was a new commit but I did not know how to go in and check if it had previous commits squashed). I'm surprised that the commits did not appear to be squashed on your end.

My upstream pointed to https://github.com/Qiskit/qiskit-terra.git. earlier. Now I've made it point to git@github.com:Qiskit/qiskit.git.

The patch file method did not work because the patch file was blank. I read that this indicates that there is no difference between those branches.

I feel rebasing is quite hard with a large number of commits present which give a lot of conflicts while interactive rebasing.

wshanks · 2023-09-12T16:27:30Z

The patch file method did not work because the patch file was blank. I read that this indicates that there is no difference between those branches.

Hmm, there are differences, as long as you didn't reset your local branch to upstream/main.

If you want, you can try generating the patch again and try to figure out why it doesn't show your changes.

If you want to skip the git diff part, I am attaching the output patch file that I get with this message.
10360.patch.txt

SameerD-phys · 2023-09-19T22:05:18Z

Hi, I ran these commands. I'm not sure if it worked this time around although I've been trying to push the releasenotes with the suggested changes for quite some time now.

If the issue still persists, is there a possibility of resolving it on your end?

This method allows applying the sign operation to an expression with possibly unassigned parameters.

wshanks · 2023-09-20T03:46:15Z

Okay, I force pushed a commit with the patch that I shared above with your information for the commit author that matches what the CLA check accpets. This restores the state to the last state you had pushed changes to. I still had some suggestions in-line for the release notes that are valid. Can you address them? You can just accept and commit my suggestions if you want.

A685

…3dd69103dc8c.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

wshanks

All good now, thanks for working on this!

jakelishman

(proxying Will's approval, since the PR touches files Will's not codeowner on)

coveralls · 2023-09-21T19:11:57Z

Pull Request Test Coverage Report for Build 6265770562

4 of 6 (66.67%) changed or added relevant lines in 1 file are covered.
19 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.01%) to 87.267%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/parameterexpression.py	4	6	66.67%

Files with Coverage Reduction	New Missed Lines	%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%
crates/qasm2/src/lex.rs	5	91.41%
crates/qasm2/src/parse.rs	12	97.13%

Totals
Change from base Build 6264759053:	-0.01%
Covered Lines:	74319
Relevant Lines:	85163

💛 - Coveralls

SameerD-phys requested a review from a team as a code owner August 4, 2023 16:50

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 4, 2023

wshanks mentioned this pull request Aug 4, 2023

Support np.sign for parameter #10360

Closed

jakelishman linked an issue Aug 4, 2023 that may be closed by this pull request

Support np.sign for parameter #10360

Closed

wshanks reviewed Aug 4, 2023

View reviewed changes

Raghav-Bell mentioned this pull request Aug 22, 2023

Negative feature #10669

Closed

wshanks requested changes Aug 23, 2023

View reviewed changes

wshanks reviewed Sep 1, 2023

View reviewed changes

qiskit/circuit/parameterexpression.py Outdated Show resolved Hide resolved

wshanks reviewed Sep 1, 2023

View reviewed changes

test/python/circuit/test_parameters.py Show resolved Hide resolved

wshanks requested changes Sep 1, 2023

View reviewed changes

wshanks requested changes Sep 3, 2023

View reviewed changes

SameerD-phys requested review from alexanderivrii, ShellyGarion, eggerdj, jyu00, woodsp-ibm, ikkoham and ElePT as code owners September 7, 2023 20:01

SameerD-phys force-pushed the negative_feature_10360 branch 2 times, most recently from 981c470 to d751c5d Compare September 8, 2023 19:08

SameerD-phys force-pushed the negative_feature_10360 branch from fe5b261 to 981c470 Compare September 8, 2023 21:40

SameerD-phys requested review from t-imamichi and nonhermitian as code owners September 19, 2023 22:03

wshanks force-pushed the negative_feature_10360 branch from 53e6676 to 5eaba01 Compare September 20, 2023 03:41

Add sign() method to ParameterExpression

0c318c2

This method allows applying the sign operation to an expression with possibly unassigned parameters.

wshanks force-pushed the negative_feature_10360 branch from 5eaba01 to 0c318c2 Compare September 20, 2023 03:42

SameerD-phys and others added 4 commits September 20, 2023 11:11

Update releasenotes/notes/added-sign-feature-parameterexpression-5769…

fb398e4

…3dd69103dc8c.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Update releasenotes/notes/added-sign-feature-parameterexpression-5769…

6ba6e2b

…3dd69103dc8c.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Update releasenotes/notes/added-sign-feature-parameterexpression-5769…

c15b1e3

…3dd69103dc8c.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Merge branch 'main' into negative_feature_10360

cc27b8f

wshanks added the Changelog: New Feature Include in the "Added" section of the changelog label Sep 21, 2023

wshanks approved these changes Sep 21, 2023

View reviewed changes

wshanks enabled auto-merge September 21, 2023 18:42

jakelishman approved these changes Sep 21, 2023

View reviewed changes

wshanks added this pull request to the merge queue Sep 21, 2023

Merged via the queue into Qiskit:main with commit b483a76 Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#10360 New sign feature in parameterexpression.py #10571

#10360 New sign feature in parameterexpression.py #10571

-        from sympy import sign
-        val = self._symbol_expr
-        sign_value = sign(val)
-        return sign_value
+        if _optionals.HAS_SYMENGINE:
+            import symengine
+            return self._call(symengine.sign)
+        else:
+            from sympy import sign as _sign
+            return self._call(_sign)

	Introduced a new feature that adds support for calculating the sign of the Parameter of the value of an expression of a Parameter class.
	Added support for expressing the sign of a :class:`ParameterExpression`.

	Instead of using numpy or other libraries, the user can use the instance of the ParameterExpression class to calculate the sign.
	Instead of assigning a concrete value and using ``numpy.sign`` or other library functions, the user can use the instance of the ParameterExpression class to calculate the sign and can work with the sign before the expression is fully assigned.

#10360 New sign feature in parameterexpression.py #10571

#10360 New sign feature in parameterexpression.py #10571

Conversation

Summary

Details and comments

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pull Request Test Coverage Report for Build 6265770562

💛 - Coveralls