-
Notifications
You must be signed in to change notification settings - Fork 1
TEBD merging 3 options #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
files of importance: 1. quimb.py 2. eval_qu.py 3. usage.py - Run for TEBD integrated with qibotn (Currently returns None) 4. nonintegrated.py - Run for self-experiments 5. test-quimb_backnd_tebd.pt - Test file for correctness sv versus tn 6. check_indep.py - Run to use the tebd fn inside eval_qu without setting backend
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
alecandido
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments just on code-style, since it's easier to start from there.
I will then comment on the substance later on.
| global TEBD_enabled | ||
| global TEBD_option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is pretty simple: never use global anywhere.
There are special cases in which you might want to abuse the module system and make a smart use of the global keyword.
Well, even in those cases you could apply the principle
Special cases aren't special enough to break the rules.
https://peps.python.org/pep-0020/
However, especially if you're not redesigning the whole library, please always avoid the use of global statements.
|
|
||
| global TEBD_enabled | ||
| global TEBD_option | ||
| TEBD_enabled = runcard.get("TEBD_enabled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is internal, no need to match 1-to-1 the option name. Thus, just follow the conventional naming rules
| TEBD_enabled = runcard.get("TEBD_enabled") | |
| tebd_enabled = runcard.get("TEBD_enabled") |
| global TEBD_option | ||
| TEBD_enabled = runcard.get("TEBD_enabled") | ||
| tebd_enabled_value = runcard.get("TEBD_enabled") | ||
| TEBD_option = runcard.get("TEBD_option") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same of above
| global TEBD_enabled | ||
| global TEBD_option | ||
| TEBD_enabled = runcard.get("TEBD_enabled") | ||
| tebd_enabled_value = runcard.get("TEBD_enabled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have two identical variables?
I'm pretty sure you can throw away one between TEBD_enabled and tebd_enabled_value, just replacing the occurrences other than definition with the variable name you decide to keep.
| if TEBD_enabled: | ||
| nqubits = circuit.nqubits | ||
| if TEBD_option == True: | ||
| state = eval.tebd_tn_qu(circuit, self.tebd_opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being TEBD an evolution approximation, I'd hope that it should not need a dedicated evaluation. Possibly the same should be true for MPS, since in both cases the network generated will differ from the dense vector with just gates application, but once you got the network, you should evaluate the resulting observable(s) in a completely analogue way.
I'm not sure whether the information that you have a specific type of network may be used or not to improve the contraction. But, if yes, I'd propagate separately from the network generation (like maintaining an MPS/TEBD flag in the network representation, or something like that).
| uni = circuit.unitary() | ||
| h = np.divide((np.log(uni)), -1 * i * dt) | ||
|
|
||
| from qibo import hamiltonians |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above: imports top-level
| if hamiltonian == "TFIM": | ||
| ham = hamiltonians.TFIM(nqubits=nqubits, dense=False) | ||
| elif hamiltonian == "NIX": | ||
| ham = hamiltonians.X(nqubits=nqubits, dense=False) | ||
| elif hamiltonian == "NIY": | ||
| ham = hamiltonians.Y(nqubits=nqubits, dense=False) | ||
| elif hamiltonian == "NIZ": | ||
| ham = hamiltonians.Z(nqubits=nqubits, dense=False) | ||
| elif hamiltonian == "XXZ": | ||
| ham = hamiltonians.XXZ(nqubits=nqubits, dense=False) | ||
| elif hamiltonian == "MC": | ||
| ham = hamiltonians.MaxCut(nqubits=nqubits, dense=False) | ||
| else: | ||
| raise_error(NotImplementedError, "QiboTN does not support custom hamiltonians") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could even rely on Qibo names, and ask for them in your runcard. In which case, you could just do:
| if hamiltonian == "TFIM": | |
| ham = hamiltonians.TFIM(nqubits=nqubits, dense=False) | |
| elif hamiltonian == "NIX": | |
| ham = hamiltonians.X(nqubits=nqubits, dense=False) | |
| elif hamiltonian == "NIY": | |
| ham = hamiltonians.Y(nqubits=nqubits, dense=False) | |
| elif hamiltonian == "NIZ": | |
| ham = hamiltonians.Z(nqubits=nqubits, dense=False) | |
| elif hamiltonian == "XXZ": | |
| ham = hamiltonians.XXZ(nqubits=nqubits, dense=False) | |
| elif hamiltonian == "MC": | |
| ham = hamiltonians.MaxCut(nqubits=nqubits, dense=False) | |
| else: | |
| raise_error(NotImplementedError, "QiboTN does not support custom hamiltonians") | |
| ham = getattr(hamiltonians, hamiltonian)(nqubits=nqubits, dense=False) |
(if you want a custom error, instead of the AttributeError it would be generated, you could catch it and re-raise)
| nqubits = circuit.nqubits | ||
|
|
||
| init_state = init_state_tn_tebd(initial_state) | ||
| from qibo import hamiltonians |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above: imports top-level
| state = np.array(list(states.values()))[-1] | ||
| return state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to name the variable you're returning right after :)
| state = np.array(list(states.values()))[-1] | |
| return state | |
| return np.array(list(states.values()))[-1] |
(unless you have a very strong motivation to give it a name)
| def init_state_tn_tebd(initial_state): | ||
| """Creates a inital MPS from a binary string.""" | ||
|
|
||
| initial_state = qtn.MPS_computational_state(initial_state) | ||
| return initial_state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a strong reason to have a function that just calls a function?
Apart from the documentation, this function definition is 100% equivalent to:
| def init_state_tn_tebd(initial_state): | |
| """Creates a inital MPS from a binary string.""" | |
| initial_state = qtn.MPS_computational_state(initial_state) | |
| return initial_state | |
| init_state_tn_tebd = qtn.MPS_computational_state |
i.e. an alias.
To use Approach 1 (Independent function), simply set runcard to None
To use Approach 2, set TEBD_option flag to False in addition to setting TEBD_enabled with your parameters dictionary
To use Approach 3, set TEBD_option flag to True in addition to setting TEBD_enabled with your parameters dictionary
This PR merges Approach 1 and Approach 2 found in PR #63 with Approach 3 found in PR #65