Implements the structure of a general qiboml model
#20
Conversation
src/qiboml/models/abstract.py
Outdated
| nqubits: int | ||
| qubits: list[int] = None | ||
| circuit: Circuit = None | ||
| initial_state: "ndarray" = None |
There was a problem hiding this comment.
| initial_state: "ndarray" = None | |
| initial_state: npt.NDArray = None |
if you mean the NumPy object (npt = numpy.typing).
However, since I assume you want to support array-like objects, the best you can do is defining your own Protocol (if you want to enforce certain restrictions, e.g. __getitem__ presence) or just add an alias for Any (since you actually won't check anything).
There was a problem hiding this comment.
Yeah, no, this was actually just a place holder to indicate a general ndarray type, either from numpy, torch, tf or jax.
There was a problem hiding this comment.
I'd say you have two options:
- use
Any, that is equivalent to leave it empty (though an explicitAnymight have a role to say "I didn't forget") - use a
Union[npt.NDArray, tf.Tensor, ...]explicitly adding all the options
I like option 2., and I'd give it an alias of its own (the frameworks you're supporting are anyhow finite, so it's fine to enumerate them all).
However, it makes sense if you're consistently investing in typing. In which case, I'd suggest also adding a workflow with Mypy.
There was a problem hiding this comment.
I could try to set up a workflow with mypy then, should I add it to the qiboteam/workflows repo and then load it here from there?
There was a problem hiding this comment.
Worfklows could be annoying. Workflows in another repo even more.
I'd suggest going in steps: do it here, and then we move to the qiboteam/workflows.
Notice that:
- Qibosoq already has it https://github.com/qiboteam/qibosoq/blob/main/.github/workflows/mypy.yml
- there are already issues for that in
qiboteam/workflows, I've simply never taken the time of implementing because there would have been no one using it- Split analysis and tests workflows#31
- Add more analysis tools workflows#32 (
symilaris part of Pylint, might be needed only when we'll remove Pylint)
There was a problem hiding this comment.
Ok I created a qiboml.ndarray type that groups the array types of all the installed frameworks: numpy (and jax) is always there, whereas torch.Tensor and tf.Tensor are present only when installed.
Co-authored-by: Alessandro Candido <[email protected]>
Co-authored-by: Alessandro Candido <[email protected]>
BrunoLiegiBastonLiegi
changed the title
qiboml modelqiboml model
|
@alecandido @MatteoRobbiati This is more or less ready for a first complete review. The ---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
Cell In[1], line 19
17 for x in data:
18 x = tf.expand_dims(x, axis=0)
---> 19 print(model(x).probabilities())
File ~/python_envs/qibo/lib/python3.10/site-packages/keras/src/utils/traceback_utils.py:122, in filter_traceback.<locals>.error_handler(*args, **kwargs)
119 filtered_tb = _process_traceback_frames(e.__traceback__)
120 # To get the full stack trace, call:
121 # `keras.config.disable_traceback_filtering()`
--> 122 raise e.with_traceback(filtered_tb) from None
123 finally:
124 del filtered_tb
File ~/python_envs/qibo/lib/python3.10/site-packages/tensorflow/python/framework/constant_op.py:108, in convert_to_eager_tensor(value, ctx, dtype)
106 dtype = dtypes.as_dtype(dtype).as_datatype_enum
107 ctx.ensure_initialized()
--> 108 return ops.EagerTensor(value, ctx.device_name, dtype)
ValueError: Exception encountered when calling BinaryEncodingLayer.call().
Attempt to convert a value (<qibo.models.circuit.Circuit object at 0x7b69c41af730>) with an unsupported type (<class 'qibo.models.circuit.Circuit'>) to a Tensor.
Arguments received by BinaryEncodingLayer.call():
• x=tf.Tensor(shape=(1, 5), dtype=float32)If I am interpreting this correctly, tensorflow tries to cast the output of each layer to an |
|
@MatteoRobbiati is the |
Certainly, it is not a priority in any case, since heavy GPU jobs would never run on Apple (and for light jobs the CPU is enough). |
I tested it locally (only CPUs) some time ago when I was implementing it into Qibo. I never tested GPUs indeed. |
|
Moreover, |
src/qiboml/models/abstract.py
Outdated
| nqubits: int | ||
| qubits: list[int] = None | ||
| circuit: Circuit = None | ||
| initial_state: "ndarray" = None |
There was a problem hiding this comment.
I'd say you have two options:
- use
Any, that is equivalent to leave it empty (though an explicitAnymight have a role to say "I didn't forget") - use a
Union[npt.NDArray, tf.Tensor, ...]explicitly adding all the options
I like option 2., and I'd give it an alias of its own (the frameworks you're supporting are anyhow finite, so it's fine to enumerate them all).
However, it makes sense if you're consistently investing in typing. In which case, I'd suggest also adding a workflow with Mypy.
| if self.qubits is None: | ||
| self.qubits = list(range(self.nqubits)) |
There was a problem hiding this comment.
Instead of this, I believe it would be more elegant to use a ._qubits attribute, and expose a .qubits property.
There was a problem hiding this comment.
The problem I see here, though, is that then you will need to construct the layer as:
QuantumCircuitLayer(nqubits=5, _qubits=[0,1,2])
if you want to specify the qubits. I am not sure if there's any workaround with dataclasses.
There was a problem hiding this comment.
Interestingly enough, you can do it with Pydantic:
class Ciao(BaseModel):
name_: str = Field(alias="name")
@property
def name(self):
print(f"Ciao, {self.name_}")
return self.name_
c = Ciao(name="come va?")
c.name(though you can not use ._qubit, since fields are supposed to be public, and they can not contain leading underscores)
src/qiboml/models/keras.py
Outdated
|
|
||
|
|
||
| @dataclass | ||
| class QuantumModel(keras.layers.Layer): # pylint: disable=no-member |
There was a problem hiding this comment.
I still wonder whether it's needed to have a model made of layers.
You just need a Circuit an encoding and a decoding.
Moreover, the encoding is fully classical, so it could be a normal Keras/Pytorch layer, without any special relation to the Circuit (though they will be provided to be used with circuits, of course), and the decoding is just an observable.
Can't we make a:
class QuantumLayer(keras.layer.Layer):
circuit: qibo.Circuit
observable: qibo.models.Hamiltonian?
There was a problem hiding this comment.
Let's say that it is more in a ML style, and indeed as you are saying you only need encoding, circuit and decoding, that you could see as three different layers stacked on top of each other. The only difference is that here you have the flexibility to construct also the circuit modularly, seen as different layers.
Regarding the encoding, I am not sure it's fully classical, in the sense that the input is usually classical but the output should be quantum in general. Instead, the decoding could be an observable, yes, but I would like to offer the possibility of returning other things, like for examples the probabilities or the samples.
In any case the QuantumLayer you are suggesting is implemented here already, it's just called in a different way. Namely, the keras interface qiboml.models.keras.QuantumModel with an ExpectationLayer as decoding is precisely taking care of that.
There was a problem hiding this comment.
The only difference is that here you have the flexibility to construct also the circuit modularly, seen as different layers.
But this you already have: different circuits are already "layers" that you can concatenate.
Simply, there are red layers and blue layers (quantum and classical) that doesn't make sense to mix. So, to me, this is better implemented if you don't provide (and then possibly deny) the option of concatenating them. But simply keeping them as different and non-compatible objects.
There was a problem hiding this comment.
Regarding the encoding, I am not sure it's fully classical, in the sense that the input is usually classical but the output should be quantum in general.
In which sense the output is quantum? Usually you encode in parameters.
If instead you're encoding in a state, this is already cutting Qiboml out of hardware support. Much better than to have a function that is emitting a circuit (thus homogeneous to the circuit part) rather than a tensor.
My take on the goal of Qiboml is more in the direction of making Qibo compatible with machine learning, not to have an ML-practitioner-friendly interface to Qibo (since Qibo already has an interface, and doesn't need a wrapper).
There was a problem hiding this comment.
I think I am losing a bit the track here, why is it a problem to have different layers doing different things that you can stack? In the end it's just a convenient way to provide the user with built in, already implemented standard variational circuits, if they want to use custom circuits they can.
In which sense the output is quantum? Usually you encode in parameters.
Consider for example the binary or phase encoding, you have some classical inputs and you convert them in a layer of X or rotation gates. Of course the gate are represented by the index of the qubit they act on or the parameter, which are classical data, but intrinsically the encoding takes classical data and embeds it in a quantum circuit. Thus I wouldn't call it fully classical.
There was a problem hiding this comment.
Consider for example the binary or phase encoding, you have some classical inputs and you convert them in a layer of X or rotation gates. Of course the gate are represented by the index of the qubit they act on or the parameter, which are classical data, but intrinsically the encoding takes classical data and embeds it in a quantum circuit. Thus I wouldn't call it fully classical.
I would classify the X as part of the circuit, and the indexing as part of the encoding. You can not make a previous layer that is stacked before the circuit, since your encoding might also be in the middle of the circuit. In that case, it doesn't make sense to see the rest of the circuit as a separate layer from the encoding.
| from qiboml.models.abstract import QuantumCircuitLayer | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
Pay attention with dataclasses that are also subclasses, they might be fragile.
The motivation is that @dataclass is generating the __init__ automatically, so it might mess up with default parameters and miss the call to super() (that you're adding in the __post_init__).
At some point, it could be just convenient to have a normal class.
There was a problem hiding this comment.
Mmmh ok, maybe then I'll only keep the interfaces (QuantumModel) as @dataclass, I'll think about it.
| def call(self, x: tf.Tensor) -> tf.Tensor: | ||
| if self.backend.name != "tensorflow": | ||
| if self.backend.name == "pytorch": | ||
| self.backend.requires_grad(False) | ||
| x = self.backend.cast(np.array(x)) |
There was a problem hiding this comment.
Can't you receive directly an array in the framework you're using?
There was a problem hiding this comment.
This is implementing the keras interface, meaning that you expect as input and output a tf.Tensor, however your backend could be any of the qibo backends. For this reason you need casts if you are not using the TensorflowBackend.
There was a problem hiding this comment.
I have the feeling you're still using the keras inside TensorFlow, and not the separate Keras distribution...
There was a problem hiding this comment.
https://keras.io/api/layers/core_layers/input/
It has no framework (nor content) associated.
| ) | ||
|
|
||
| def forward(self, x: torch.Tensor): | ||
| if self.backend.name != "pytorch": |
There was a problem hiding this comment.
Any reason to use PyTorch with a non-Pytorch backend?
There was a problem hiding this comment.
Similarly to the comment above, this is the pytorch interface, which consumes torch.tensors but this is separated from the backend you are using. If the backend is the PyTorchBackend, then you don't need to do anything, otherwise you have to cast to the appropriate type. For example, if you want to run on hardware using the pytorch framework, you are going to use the QibolabBackend.
There was a problem hiding this comment.
This is the role of the evaluate() function, to interface the front-end and back-end (gradients included).
I would argue that the conversion should happen in a single place (and that's evaluate(), that is working for every (frontend, backend) pair).
Instead, the model should just play with the front-end. Thus, if you're using a PyTorch model, you can safely assume your front-end is PyTorch, otherwise is good to just fail (and leave the back-end conversion somewhere else).
There was a problem hiding this comment.
Yes indeed, but since there is currently no implementation of the evaluate function, as gradients are still not in main yet, I had to take care of that here in order to test. As I said in one of the previous meetings, this object is probably going to change when we incorporate the auto differentiation, as pytorch wants you to write a custom autograd function which is probably going to perform what is done here in the forward.
There was a problem hiding this comment.
Ok, maybe it's worth to commit a bit of time to the Pytorch implementation, since (in principle) expectation() is in main:
qiboml/src/qiboml/operations/expectation.py
Lines 12 to 19 in 0e906ee
(sorry, I messed up, it's not
evaluate())
Working on top of that, should make the model structure more compatible.
In any case, if you prefer doing it in a different PR I'm not completely against. But you might end up reverting there part of what is done here.
There was a problem hiding this comment.
Just one simple criticality spotted top-level.
About the longer-standing discussions, there is no need to solve in this PR anyhow. Qiboml is an experimental module, and it will keep being experimental even after the PR.
We should take care about having a programmatic discussion before more serious attempts towards stabilization, but it is not the yet the time for this. Keep experimenting is fine, and moving faster is definitely a priority (though collecting observations along the why is good, as well as keep discussing them - and we should still pay attention to more prosaic software aspects, as correct import/dependencies, and of course tests :P).
| ndarray = npt.NDArray | ||
|
|
||
| try: | ||
| from tensorflow import Tensor as tf_tensor | ||
|
|
||
| from qiboml.models import keras | ||
|
|
||
| ndarray = Union[ndarray, tf_tensor] | ||
| except ImportError: | ||
| pass | ||
|
|
||
| try: | ||
| from torch import Tensor as pt_tensor | ||
|
|
||
| from qiboml.models import pytorch | ||
|
|
||
| ndarray = Union[ndarray, pt_tensor] | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
You should never import from a level directly above yours, and the package __init__.py is directly above everything.
I'd suggest you to move this definition in a standalone module (though I still a bit undecided about which would be the best course of action for this...).
This PR is drafting a possible structure for the
qibomlmodels. Namely an abstractQuantumCircuitLayeris implemented, which all the other models are going to subclass.Three interfaces are going to be provided for this
qibomlmodel, namely apytorch,tensorflowandjaxinterface.