Enhancement of converters of QuadraticProgram

[t-imamichi]

What is the expected enhancement?

I would like to discuss how the converters of QuadraticProgram should behave in the next version.
There are some issues related the converters of QuadraticProgram, #1032 and #982.

• QuadraticProgramToQubo does not include InequalityToEquality converter. So, users need to apply it manually.
  • Shall we include InequalityToEquality in QuadraticProgramToQubo?
  • The error message of QuadraticProgramToIsing is not so informative. It would be nice to recommend relevant converters to the error.
• Penalty factor of LinearEqualityToPenalty is user-defined parameter and the default value is too big for some use cases (such as Grover)
  • We can minimize the penalty factor to guarantee the optimal solution satisfies the constraints. We implemented the functionality in the previous version; but it was not imported to the current version. We can import it.
  • We would like some algorithms to control the factor. How to realize it?
• If all coefficients of variables are integer, rhs of inequality constraints will be rounded up/down in InequalityToEquality. It would be nice to mention it in the tutorial.
• If QuadraticProgram contains continuous variables, we cannot apply VQE/QAOA directly. It would be nice to recommend ADMM as part of a warning message.

[t-imamichi]

#1043 addresses the penalty factor

[t-imamichi]

If converters does not support the input QuadraticProgram, the error message should suggest which converter to apply beforehand.
Or, it would be an option that QuadraticProgramToIsing applies necessary converters automatically.

[woodsp-ibm]

Some thoughts....
I made a comment in #1043 around convertor re-use. I imagine given I setup a convertor I would be able to use it more than once to do a conversion of some input to the output format. As such whatever parameters are setup for the convertor, via the constructor, care should be taken to preserve these instance vars storing them, and not alter them, so each conversion is as expected.

Also I wonder if it would be better not to store src and dst in these convertors as instance vars, if the convertor instances are expected to be reused. Rather keep src/dst just as local vars in the encode and any supporting functions so they exist only during the conversion.

[t-imamichi]

I see. You prefer a pure function like converter to reuse it in other places once instantiated.

I designed to store src and dst and associate the converter with a QP in order to implement decode as well as encode. Given a result, decode is supposed to transform it into a result corresponding to the original QP. If we keep the information QP somewhere else, we make converters like pure functions.

[t-imamichi]

By the way, I discussed this topic with @stefan-woerner. We agree the followings:

1. include InequalityToEquality to QuadraticProgramToQubo,
2. revise error message to suggest a missing converter, and
3. write a description about round up/down of InequalityToEquality in a tutorial.

We also discussed to distinguish "converter" (QP <=> QP) and "translator" (QP <=> other, e.g., Operator). It might be intuitive to make QPtoIsing translator from QP to Operator. I want to discuss more about converter re-use in the context.

[woodsp-ibm]

I see. You prefer a pure function like converter to reuse it in other places once instantiated.

My comment was more around an end-user expectation of what they might intuitively expect of a converter. If all the config for a convertor is in the constructor and encode takes the object I want to convert and gives me a converted object back, then to me it does not seem unreasonable to believe an end user would expect to be able to reuse the convertor under the same configuration as was passed in. This came about as I noticed in #1043 that the input penalty would end up being overidden in auto case by the computed penalty - any re-use would then use this first computed penalty. Perhaps this is a some use case to convert a set under the same auto penalty as the first value.

In terms of src and dst and storing it so it can be decoded. In the latter it seems that the original object is modified rather than a new converted instance being returned. Perhaps the convertors should all be consistent in returning a new object and not modifying the original in place (or all modifying the original). I think having different behaviors among the convertors will cause users to end up with unexpected problems around this.

So for me it is more to be consistent and do intuitively what you might expect. If there is some behavior that seems it would not be so then we should make a Note: in the documentation, though sometimes these are not read so much so intuitive behavior of what you might expect does seem preferable.

[t-imamichi]

If converters keep src and dst, it is easy to convert the result of dst to the result of src. But, I agree that it might not be so intuitive to users. Especially, users may pass wrong result to decode by reusing converters. In such a situation, they will receive a wrong result, and it is hard to debug it. Ideally, converters should decode result without information of src. If we append some information to the encoded problem, we may be able to decode the result. Perhaps, it might be possible with no extra information. I would like to think of the idea.

[t-imamichi]

Feel free to write your idea about the converter design. @adekusar-drl

[t-imamichi]

Idea about metadata

It is necessary to keep some metadata about the original problem to realize decode of results, e.g., ignore slack variables and combining binary variables into an integer variable.
The current design keeps the metadata in the converters. The other options are: to keep the metadata in the problems after conversion, and to keep the metadata as a separate data.
I don't think it's a good idea to expect users to manage metadata correctly. So, they may combine wrong metadata and result when decoding.
So, I think of a following flow.

1. when encode of QuadraticProblem to QuadraticProblem, a converter appends the metadata to the output problem
2. when solve QuadraticProblem to obtain OptimizationResult, an algorithm copies the metadata to the internal of the result
3. when decode of OptimizationResult to OptimizationResult, a converter handle variables in the result according to the metadata embedded in the result itself.

I will think of ideas about treating parameters of converter as property.

[woodsp-ibm]

If encode provided a new object back, that was encoded version of the original which is left untouched, rather than doing the encode in-place, there would be no real need for decode of the last encoded object, i.e. the in-place altered one, since you could keep the original too should it be needed. Maybe I am missing an understanding of exactly the use case for decoding the last encoded object that the convertors look like they do.

[adekusar-drl]

@t-imamichi Thanks for adding me to this discussion. I explored the classes from the converters package and here are a few my findings/suggestions:

• Converters, in general, don't inherit any base type. Probably it is good to have a base class for them. If we prefer duck-typing, this is still fine, but should be mentioned somehow in the documentation and strongly followed in all implementations.
• Their signatures are pretty different. I suggest to put parameters to constructors and have encode with only one parameter, the problem itself.
• Some converters have only encode without decode. Are these also converters? Shall we split them into two basic types, e.g. converters have both encode/decode and, say, transformers have only encode. Moreover, there are separate IsingToQuadraticProgram and QuadraticProgramToIsing and from the first impression it is not clear why they are converters and why separate.
• encode should return a new problem unless it is stated as inplace=True in the constructor and it is feasible. This behavior should be also well documented I believe.
• Converter metadata should be kept in the converter. You never know what can be done with the problem object after encode is called.
• I'd prefer to see something that prevents me from forgetting to call decode at the end. E.g. something like chaining converters: with_converter(IntegerToBinary).solve(qp) or with_converter(c1).with_converter(c2).solve(qp). If this is not a pythonic way, sorry for that.
• I don't mind of keeping converters expendable. The problem is that if a converter has only one method, encode, you likely can re-use it but if a converter has both encode and decode, then likely it is stateful and you can't reuse it.
• If an optimizer uses a converter in the solve and the converter requires parameters, I'd suggest to expose converter's parameters in optimizer and don't expose access to the converter from the optimizer.

Feel free to criticize.

[t-imamichi]

@woodsp-ibm encode is a conversion from QuadraticProgram to QuadraticProgram. On the other hand, decode is a conversion from OptimizationResult to OptimizationResult. So, just keeping the original QuadraticProgram is not sufficient to decode OptimizationResult. Let me think more about the data flow...

[t-imamichi]

@adekusar-drl Thank you for your feedback.

• I agree with the base class. My initial design proposed a base class, but it dropped when implementing the entire stack. By distinguish conversions QP <=> QP and QP <=> opflow, a base class may work well. I'm thinking of the following base class now taking into account your second comment about signature.

class BaseConverter:
  def __init__(param):
    self._param = param
  def encode(prog: QuadraticProgram) -> QuadraticProgram:
    pass
  def decode(result: OptimizationResult) -> OptimizationResult:
   pass

• As for third comment, @stefan-woerner and I agree that converter should work for only between QuadraticProgram and we have to make QuadraticProgramToIsing and IsingToQuadraticProgram different name such as "Translator". They are the main reason we dropped the base class because a conversion between QP and Operator do not have decode.
• I agree that the in-place mode should be optional
• Chaining converters sounds nice. I would like to think of how to realize it.
• As for metadata, we have an option to keep it in the resulting QP. Then, we know what's done with the problem. I try to make a PoC of the design.
• I'm afraid that I don't get the last comment. What do you mean "don't expose access to the converter from the optimizer."? If an optimizer uses a converter in solve, what the optimizer can access and cannot access?

[t-imamichi]

I will soon make a PoC code to discuss more details.

[adekusar-drl]

@t-imamichi

• Here is an example against keeping metadata in the resulting QP. In a code you apply a converter to a QP, so after that a metadata from the first converter is in the output QP. Then you apply another converter. If you don't keep an on eye on metadata in your code, you may loose the metadata from the first converter when you apply the second one.
• Regarding "don't expose access to the converter from the optimizer". If I recall correctly, there was a discussion how to setup converters in optimizers, e.g. how to pass parameters to a converter that is used in an optimizer. And once again if I recall correctly somebody suggested to have a property for the converter and tune that converter by accessing its properties. I think it is better to add additional properties to the optimizer's constructor itself and then pass them to the converter. Hope this makes it clear.

[t-imamichi]

• If we embed metadata in QP, converters will be implemented taking into account the metadata. They should not overwrite metadata, but append it. When decoding, converters decode results according to the metadata. I'm trying to make a PoC.
• I get your point. It's @stefan-woerner's idea. He want to treat the penalty factor as a property from Grover optimizer. I understand that he wants to change the factor iteratively in the algorithm. I would like to think of the details.

[adekusar-drl]

There may be any code (e.g. user code) after a converter has been applied that modifies the problem in any way you can't predict. If you append metadata to QP, then you rely on the code you can't control and this is error prone.

[t-imamichi]

We have the same issue when embedding the metadata in converters as Steve mentioned. Users may reuse converters embedded with some metadata to different QP in the current design. It causes wrong decoding results.

[adekusar-drl]

I know, I think converters should be expendable. Anyway, if encode is being called for the second time without a call to decode you may prevent this and raise an exception.

[t-imamichi]

As a summary, we have to make converters more intuitive and fail safe.

• Distinguish converters and translators
  • Converters: (encode) QP => QP, (decode) Result => Result
  • Translator: (encode) QP <=> Operator
• Need to take care of following situation to be fail safe
  • Reuse converters
  • Apply same type of converter multiple times
  • Apply decoders in a wrong order
• Question: How to prevent metadata to be removed or overwritten?

[stefan-woerner]

Thanks, I agree on most of the points above!

Regarding accessing converter parameters from an algorithm, I think it is very important, but I am fine exposing the parameters instead of the converters, just needs to be done consistent.
In addition, we might want to allow users to provide their own (list of) converter(s) that are used instead of the default ones.

On the meta data discussion for converters, I think in most cases we have the use case that we encode/decode within an algorithm. Thus, just raising a warning if decode has been skipped sounds like a good and pragmatic idea.

[woodsp-ibm]

encode is a conversion from QuadraticProgram to QuadraticProgram. On the other hand, decode is a conversion from OptimizationResult to OptimizationResult.

'encode' and 'decode' seem more like some symmetric behavior according to the naming. Let me say what these objects do then in a different way - they 'translate' the problem from one form into another, and then you need a method to 'interpret' the result of solving the problem in the context of the translated problem so it can be expressed in the form of the original one. I think the fact that one half of the convertor acts on the problem and the other half acts on a result object could be made clearer perhaps with better naming.

Maybe 'encode' is fine - 'convert', 'translate', even 'encode_problem' would be possible here too I think. For the other 'interpret', 'interpret_result', 'process_result' or whatever. Maybe I looked at the object too casually and did not pay enough attention to datatypes - but I feel that something other than what looks like it does symmetric, and potentially reversible operation, like having encode and decode as names may help to dispel such potential confusion.

One other thought may be to have the convertor just have the encode method and then another class, maybe in that same convertor be (also) returned from the 'encode'. This new instance would store whatever it needed to interpret the final result so you would call a method on this to do so. This now makes the original convertor re-usable since it does not store any data itself for any result interpretation down the line rather that's in an object that is passed back which the caller stores and uses of course.

[t-imamichi]

@woodsp-ibm Yes, I agree. decode does not sound as intuitive as encode and the asymmetric functionality of encode and decode might be confusing. Separating them would be a better option. Maybe it would be good to have only one-way converters, i.e., QP->QP, QP->Operator, Operator->Result, and Result->Result.

[woodsp-ibm]

@t-imamichi Different classes occured to me too since these conversions are acting on different types. But since it was indicated that the decode function, to process a result from a given problem conversion, depended on data/info from that original problem conversion, it was how I came up with the above thought. I.e. that the problem convertor class, could contain another class for result conversion in that same module, that the problem convertor knows how to create when it does the problem conversion and passes an instance back, complete with whatever data/info it needs, so that instance later it can be called upon to do a result conversion.

[a-matsuo]

I'm working on this issue in PR #1061. About the name of the argument for QuadraticProgram in encode method, I feel problem sounds more intuitive for the converter instead of prog. What do you guys think? (It's no big deal. I just felt so. Since the class name is QuadraticProgram, prog might be better).

[adekusar-drl]

I'm ok with this. I use problem as well.

[t-imamichi]

@woodsp-ibm pointed out that Aqua chemistry deal with conversion of problems in Hamiltonian class and results as follows.

• Hamiltonian.run(qmolecule) converts molecule data into qubit operator. https://github.com/Qiskit/qiskit-aqua/blob/5312359e94b105f8122e0660974be60b6a3450a2/qiskit/chemistry/core/hamiltonian.py#L119
• Hamiltonian._process_algorithm_result(algo_result) converts the results. https://github.com/Qiskit/qiskit-aqua/blob/393bf2d4b22c2414fbbf5e41d540601c0cf63bbd/qiskit/chemistry/core/hamiltonian.py#L363
  • It is invoked from ChemistryOperator. https://github.com/Qiskit/qiskit-aqua/blob/393bf2d4b22c2414fbbf5e41d540601c0cf63bbd/qiskit/chemistry/core/chemistry_operator.py#L64
  • It copies some class variables, e.g., self._hf_energy this is copied from qmolecule, to the result object. https://github.com/Qiskit/qiskit-aqua/blob/393bf2d4b22c2414fbbf5e41d540601c0cf63bbd/qiskit/chemistry/core/hamiltonian.py#L122
    https://github.com/Qiskit/qiskit-aqua/blob/393bf2d4b22c2414fbbf5e41d540601c0cf63bbd/qiskit/chemistry/core/hamiltonian.py#L376

I come up with the following base class of converters between QuadraticProgram. What do you think?
Translations between different classes such as QP to Ising are not target.

class QuadraticProgramConverter:
  # Base class of converters between QuadraticProgram.
  # To be clear of conversion between QuadraticPrograms,
  # it might be good to include 'QuadraticProgram' in the base class name

  def __init__(value) -> None:
    pass

  def convert(self, problem: QuadraticProgram) -> QuadraticProgram:
    # convert a problem into another form and keep some information required to interpret the result
   pass

  def interpret(self, result: OptimizationResult) -> OptimizationResult:  # interpret or process or evaluate
    # interpret a result into another form using the information of conversion
    # E.g., ignore values of slack variables appended by `convert`.
    pass

class ConverterExample(QuadraticProgramConverter):
  def __init__(value) -> None:
    self._param: int = value  # parameters of converter.
    self._metadata = {}  # such as list of slack variables appended and binary variables to encode integer variables
    # Names `_param` and `_metadata` are just examples. Developers can choose anything as they want.
    # Developers can keep the input and output problems if necessary to interpret results.

  @property
  def param(self) -> int:
    return self._param

  @param.setter
  def param(self, value: int):
    self._param = value

  def convert(problem):
    new_problem = ...
    ...
    return new_problem

  def interpret(result):
    new_result = ...
    ...
    return new_result

[stefan-woerner]

@t-imamichi I like your suggestion!

[adekusar-drl]

@t-imamichi Imamichi-san, looks nice.

[t-imamichi]

Thank you for your responses. I will make a PR based on my idea.

[stefan-woerner]

I think we should move the QuadraticProgramToNegativeValueOracle into a algorithms / grover_optimizer folder together with the GroverOptimizer as we also do for other algorithms in aqua. Eventually, this should go to the circuit library as a quantum arithmetic circuit to evaluate quadratic functions, but that requires the refactoring of the base Grover algorithm first.

[a-matsuo]

I changed the following things based on the above discussion and Imamichi-san's document in #1061 (I haven't changed anything of QuadraticProgramToNegativeValueOracle).

• introduced QuadraticProgramConverter which is an abstract class for converters
• renamed encode/decode to convert/interpret
• moved all parameters to __init__
• created setter/getter for converter parameters
• enabled “auto_penalty” (added LinearEqualityToPenalty._auto_define_penalty() function #1043)

A few things (interpret of QuadraticProgramToQubo needs interpret of LinearEqualityToPenalty) are left which has dependency on #1043, but basically it's done. If you need any other things, please tell me or add to PR's code.

[t-imamichi]

I will work on the change of converters with @a-matsuo on branch #1061