Skip to content

Merge condition check codes in compose() method#6983

Merged
mergify[bot] merged 5 commits into
Qiskit:mainfrom
QianJianhua1:compose
Oct 1, 2021
Merged

Merge condition check codes in compose() method#6983
mergify[bot] merged 5 commits into
Qiskit:mainfrom
QianJianhua1:compose

Conversation

@QianJianhua1
Copy link
Copy Markdown
Contributor

Summary

Merge condition check codes in compose() method.

Details and comments

Merge the two consecutive "if front" check codes into one.

@QianJianhua1 QianJianhua1 requested a review from a team as a code owner September 6, 2021 02:11
Copy link
Copy Markdown
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Thanks @QianJianhua1 , I agree these two if statements could be merged (though I would expect the performance improvement in doing so to be small). However, to me, I find the updated version less readable than the original (having this as two conditions did separate out the "merge _data lists" phase from the "update ParameterTable" phase, and the comment # just append new parameters is now misplaced). Is there a way the readability of the original could be preserved?

@QianJianhua1
Copy link
Copy Markdown
Contributor Author

Thank you @kdk Partly agree with you. But if you think of "update _data" and "update parameter" as one continuous process, you might think otherwise. In other words, after _data is updated, parameter must be updated.
if front is true, we just consider how to deal with them(contain _data and parameter), Otherwise just append the new part.
I have changed the comments, please review it again.

@javabster javabster added the Changelog: None Do not include in the GitHub Release changelog. label Oct 1, 2021
Copy link
Copy Markdown
Contributor

@javabster javabster left a comment

Choose a reason for hiding this comment

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

This lgtm, thanks @QianJianhua1!

@jakelishman
Copy link
Copy Markdown
Member

On this one I agree with @QianJianhua1 about the newer readability: in the previous version with its split if statements, it wasn't super clear why you needed to invalidate the parameter table in the second if front, unless you also saw the first if front. Putting them together, it's very clear to me - we have to invalidate it because data has been inserted before, so all previous indices are now invalid.

Copy link
Copy Markdown
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @QianJianhua1 , this LGTM!

@mergify mergify Bot merged commit 0c27433 into Qiskit:main Oct 1, 2021
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
@QianJianhua1 QianJianhua1 deleted the compose branch December 24, 2021 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in the GitHub Release changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants