Various fixes to the differentiation module #62
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
==========================================
- Coverage 87.90% 87.85% -0.06%
==========================================
Files 11 11
Lines 488 494 +6
==========================================
+ Hits 429 434 +5
- Misses 59 60 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Thanks again @BrunoLiegiBastonLiegi for this!!
I just added some comments below.
Let's discuss them quickly and - once this is done - for me we are good to go.
| @@ -16,6 +16,7 @@ | |||
| "qiboml-pytorch": None, | |||
| "qiboml-tensorflow": Jax, | |||
| "qiboml-jax": Jax, | |||
| "numpy": Jax, | |||
There was a problem hiding this comment.
Are we supposing here a user could configure the Numpy backend?
There was a problem hiding this comment.
yes since qibo is a dependency, and numpy its native backend.
There was a problem hiding this comment.
yes since
qibois a dependency, andnumpyits native backend.
If Qibojit installed, the default should be Qibojit, no?
| @@ -32,8 +35,14 @@ def circuit( | |||
| ): | |||
| return self._circuit | |||
|
|
|||
| @property | |||
| def differentiable(self): | |||
There was a problem hiding this comment.
Isn't maybe safer to set this property as False by default (1) or we assume cases like binary encoder are more exceptional while the default should in general be differentiable (2)?
There was a problem hiding this comment.
I would say it's more common to have a differentiable encoding.
| ) | ||
| ) | ||
| else: | ||
| gradient.append(backend.np.zeros((decoding.output_shape[-1], x.shape[-1]))) |
There was a problem hiding this comment.
I am not sure we can just pad the gradient vector with values or zeros here.
It is probably safe until the model structure is: encoding -> circuit -> decoding, but as soon as we will have a mix of trainable and non trainable gates we should probably loop over the gates.
I say this because I guess the position of the gradients in the list is important, while here we are basically padding it at its beginning.
There was a problem hiding this comment.
In our design right now, the encoding is never trainable and all its gates are in the beginning. If the design will change, this will be updated accordingly, but I cannot modify this now predicting how the design will change.
This implements some fixes to differentiation:
differentiablepropertyJaxby usingfunctools.partialinstead of class attributes to avoid leakage