-
Notifications
You must be signed in to change notification settings - Fork 129
Added DFL entropy code #444
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
Conversation
NoureldinYosri
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.
recirq/dfl/dfl_entropy.py
Outdated
| moment = [] | ||
| for i in range(len(grid)): | ||
| if i % 2 == 0: | ||
| if matter_config.value == InitialState.SINGLE_SECTOR.value: |
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.
nit here and below, there is no need to call .value
| if matter_config.value == InitialState.SINGLE_SECTOR.value: | |
| if matter_config == InitialState.SINGLE_SECTOR: |
recirq/dfl/dfl_entropy.py
Outdated
| ValueError: If initial_state is not valid. | ||
| """ | ||
|
|
||
| if initial_state == "single_sector": |
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.
the if condition can be replaced by one line
| if initial_state == "single_sector": | |
| initial_circuit = initial_state_for_entropy( | |
| grid, getattr(InitialState, initial_state.upper())) |
dstrain115
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.
Could we get some tests for the circuit generation part of this at least.
Also, I would run these files through "black" since the formatting looks pretty goofy overall.
| def _layer_interaction(grid: Sequence[cirq.GridQubit], | ||
| dt: float,) -> cirq.Circuit: | ||
| """Implements the ZZZ term of the DFL Hamiltonian | ||
| Args: |
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.
nit: maybe explain what this circuit is supposed to look like?
Also put a blank line before "Args:"
recirq/dfl/dfl_entropy.py
Outdated
|
|
||
| def _layer_matter_gauge_x( | ||
| grid: Sequence[cirq.GridQubit], dt: float, mu: float, h: float) -> cirq.Circuit: | ||
| """Implements the matter and gauge fields |
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 here. It would be good to explain the relationship between "matter and gauge fields" and the generated circuits.
recirq/dfl/dfl_entropy.py
Outdated
| h: float, | ||
| mu: float, | ||
| n_basis: int = 100) -> Sequence[cirq.Circuit]: | ||
| """Generate the circuit instances the entropy experiment |
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.
It seems like there is a word missing here. Is it supposed to be "Generate the circuit instances (for) the entropy experiment"?
recirq/dfl/dfl_entropy.py
Outdated
| """Generate the circuit instances the entropy experiment | ||
|
|
||
| Args: | ||
| initial_state: Which initial state, either "single_sector" or "superposition" or "disordered. |
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.
Should this be an InitialState object instead? Also, missing a quote at the end.
recirq/dfl/dfl_entropy.py
Outdated
|
|
||
| def run_1d_dfl_entropy_experiment( | ||
| grid: Sequence[cirq.GridQubit], | ||
| initial_states: Sequence[str], |
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.
Should this be an InitialState object instead? It seems awkeward to mix enums and strings.
| gauge_compile: bool = True, | ||
| dynamical_decouple: bool = True) -> None: | ||
| """Run the 1D DFL experiment (Fig 4 of the paper). | ||
| The paper is available at: https://arxiv.org/abs/2410.06557 |
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.
Please add a description about where this data is being saved, since it is writing to local disk.
recirq/dfl/dfl_entropy.py
Outdated
| if not os.path.isdir(save_dir + | ||
| "/dt{:.2f}/h{:.2f}_mu{:.2f}".format(dt, h, mu)): | ||
| os.mkdir( | ||
| save_dir + "/dt{:.2f}/h{:.2f}_mu{:.2f}".format(dt, h, mu)) |
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.
Can we set dir_name = f"dt{dt:.2f}/h{h:.2f}_mu{mu:.2f}" instead of restating it over and over?
Also. I think it would be better to use Path(f"dt{dt:.2f}/h{h:.2f}_mu{mu:.2f}")
recirq/dfl/dfl_entropy.py
Outdated
| for initial_state in initial_states: | ||
| if not os.path.isdir( | ||
| save_dir + | ||
| "/dt{:.2f}/h{:.2f}_mu{:.2f}/{:s}".format( |
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.
If you use Path above, then you can have dir_name / filename
recirq/dfl/dfl_entropy.py
Outdated
| initial_state)) | ||
|
|
||
| for n_cycle in n_cycles: | ||
| print(initial_state, n_cycle) |
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.
nit: It might be good to have this print statement be more descriptive rather than just two numbers
recirq/dfl/dfl_entropy.py
Outdated
| Args: | ||
| initial_state: Which initial state, either "single_sector" or "superposition" or "disordered. | ||
| ncycles: The number of Trotter steps (can be 0). | ||
| basis: The basis in which to measure. Either "x" or "z". |
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.
basis is missing from the function arguments.
No description provided.