Skip to content

Fix QuadraticProgramElement when in-place copy of QuadraticProgram#192

Merged
mergify[bot] merged 4 commits intoqiskit-community:mainfrom
t-imamichi:fix-qpe
Jul 3, 2021
Merged

Fix QuadraticProgramElement when in-place copy of QuadraticProgram#192
mergify[bot] merged 4 commits intoqiskit-community:mainfrom
t-imamichi:fix-qpe

Conversation

@t-imamichi
Copy link
Copy Markdown
Collaborator

@t-imamichi t-imamichi commented Jul 2, 2021

Summary

QP.from_ising, QP.read_from_lp_file, and QP.from_docplex copy another QP to self.
These changes were introduced in #122 #166.
I notice that I forgot to update QuadraricProgramElement.quadratic_program when the copy.
So, I fix it and add unit tests. Line 847 of QP is the core part of this PR.

I also revise type hints and error message (format to f-string) in QP.

Details and comments

self, constant, linear, quadratic, QuadraticObjective.Sense.MAXIMIZE
)

def _copy_from(self, other: "QuadraticProgram") -> None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This also changes the 'other' QP as a byproduct of doing this right? Each element has the parent updated to self so 'other' is no longer a self-consistent QP anymore. To keep them both consistent it would seem that a deepcopy of each element would need to be done, such that the parent change is just in the copy, and self is then populated with these copies. Maybe in the context of what is being done the current behavior is fine, just not sure, hence the comment.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the comment. _copy_from is used only in from_docplex, from_ising and read_from_lp_file. They generate a new QP object other by translator (e.g., from_docplex_mp) and _copy_from copies the internals of other to self. The remaining other won't be used anywhere else. So, it's safe.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I added a note so that we don't use other after the copy.

Copy link
Copy Markdown
Member

@woodsp-ibm woodsp-ibm Jul 2, 2021

Choose a reason for hiding this comment

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

Sorry, I approved and dismissed it since I wanted to double check something.

So if I look at from_ising this essentially changes the entire content of the QP you give to that of the one given back from to_ising convertor. So its like clearing out the current QP and replacing it with the one from from_ising. I guess I can expect that it updates the objective, vars and constraints. But it also replaces the name I give to whatever is in the from_ising QP. So if I do

>>> q_p = QuadraticProgram(name='mememe')
>>> q_p
\ This file has been generated by DOcplex
\ ENCODING=ISO-8859-1
\Problem name: mememe
...

and then

>>> q_p.from_ising(PauliSumOp.from_list([("IZ", 1), ("ZZ", 2), ("ZI", 3)]))
>>> q_p
\ This file has been generated by DOcplex
\ ENCODING=ISO-8859-1
\Problem name: CPLEX
...

It ends up with my custom name replaced by CPLEX

Is this something that was intended - I guess its a byproduct of copy over all the attributes which might be more than is wanted.

Update: it occurred to me later after writing the above that when creating a QP from an LP file, since it has the problem name in there, in that case I guess one might expect the name to become that from the LP file. Since an Ising representation has no name, maybe that should not change the name perhaps - that means the copy from function would need to be told whether to include the name or not if this is how we want things to behave overall when 'importing' from another format i.e ising or lp.

Copy link
Copy Markdown
Collaborator Author

@t-imamichi t-imamichi Jul 3, 2021

Choose a reason for hiding this comment

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

Good point. We should keep the problem name when from_ising. I added include_name option and check the problem name in the unit tests. 1890e73

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Btw, I notice that it seems strange that the unit tests of QP.from_ising and QP.to_ising are located in test_converters.py. This may be because from_ising and to_ising were part of converters in the past. I will decompose test_converters.py for each converter in a separate PR.

woodsp-ibm
woodsp-ibm previously approved these changes Jul 2, 2021
@woodsp-ibm woodsp-ibm dismissed their stale review July 2, 2021 17:43

Need to double check

@mergify mergify Bot merged commit 4347110 into qiskit-community:main Jul 3, 2021
@t-imamichi t-imamichi deleted the fix-qpe branch July 4, 2021 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants