redefining OperatorEvolution class and fixing typehints - #1528
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1528 +/- ##
==========================================
+ Coverage 38.90% 38.96% +0.05%
==========================================
Files 120 120
Lines 6225 6231 +6
==========================================
+ Hits 2422 2428 +6
Misses 3803 3803
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RoyStegeman
left a comment
There was a problem hiding this comment.
Different from what the title of this PR suggests, changes go beyond typehints and the interface is also being changed. In particular, OperatorEvolution now separates the static and time-dependent parts into two attributes. This change is propagated and many of the changes in this PR are to accommodate this change.
Before I continue reviewing. What is the reason for this change?
| initial_state: Operator, | ||
| time: list[float], | ||
| collapse_operators: list[Operator] = None, | ||
| time: ArrayLike, |
There was a problem hiding this comment.
in DynamiqsEngine.evolve time is an Iterable. An iterable can also be a dict which is not an ArrayLike. So this typehint is too restrictive (actually it was already before, but it still is). I'm assuming that Iterable is more general than necessary, so in that case you could change that one instead of here.
| def evolve( | ||
| self, | ||
| hamiltonian: Operator, | ||
| hamiltonian: OperatorEvolution, |
There was a problem hiding this comment.
qibolab/benchmarks/engine_sweep.py
Line 81 in dd6f942
This hamiltonian can be 0 or something else, but eventually is also passed to evolve.
OperatorEvolution class and fixing typehints
In this PR, I am redefining the emulator's data handling to make it clearer and more expressive, particularly the way static and time-dependent Hamiltonians are represented before starting the simulation.
Specifically, the
staticanddynamicalcontributions of the full Hamiltonian have been split into two separate objects: the static term is now represented by anOperator, while the dynamical terms are represented by anOperatorEvolution.The main issue with the previous design was that
OperatorEvolution.operatorsis typed aslist[Operator | TimeDependentOperator], which introduces some ambiguity. In particular, when loading data from the filesystem,maincurrently relies on the convention that the first element of this list is the static term. I believe the new design makes the data model much clearer by explicitly separating static and dynamical terms instead of encoding them within a single object.To better understand the motivation behind these changes, please also refer to the updates to the loading functions in
qutip.py.Finally, I added several type hints throughout the code.
Note that we have decided to focus exclusively on the
qutipengine. Therefore, feedback related to thedynamiqsengine is not a priority, as it may already be outdated, partially broken, and not fully tested.Checklist: