Skip to content
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

Methods for calculating expectation values from a list of observables, sampling from final state. #61

Merged
merged 23 commits into from
Jan 12, 2021

Conversation

sebgrijalva
Copy link
Contributor

@sebgrijalva sebgrijalva commented Jan 7, 2021

Changes:

  • Simulation.run() only evaluates the states at each time
  • Simulation.run() outputs a list of numpy.ndarray vectors instead of qutip.Qobj
  • To obtain expectation values, a new method Simulation.expect() allows for a list of observables to be provided, then uses the states at each time step to show the evolution of the observables.
  • A new method, Simulation.sample_final_state() returns a collections.Count dictionary with a chosen number of samples from the final state of evolution.

The notebooks have been adapted to reflect these proposed changes.

This also in reference to #55 and #56

`run()` method now only creates self.output with the state evolution and 
there's a new method `expect()` that takes a list of observables (Qobjs 
or ndarrays).
If the user wants the Qobjs, she can access them from 
`Simulation.output`.
@sebgrijalva
Copy link
Contributor Author

Comments/Reviews are very welcome, @HGSilveri , @LucasGitQ @cdalyac

Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some remarks and potential improvements.

I still think these methods would fit better in a dedicated SimulationResults class, which would be the output of Simulation.run().

pulser/simulation.py Outdated Show resolved Hide resolved
pulser/simulation.py Outdated Show resolved Hide resolved
pulser/simulation.py Outdated Show resolved Hide resolved
pulser/simulation.py Outdated Show resolved Hide resolved
pulser/simulation.py Outdated Show resolved Hide resolved
pulser/simulation.py Outdated Show resolved Hide resolved
pulser/simulation.py Outdated Show resolved Hide resolved
@lhenriet
Copy link
Member

lhenriet commented Jan 7, 2021

Ah I remark that one of my comments on N was also pointed out by Henrique. The fix still does not address the 2-level nature of the measurement vs the (occasional) 3-level nature of the system.

@HGSilveri
Copy link
Collaborator

HGSilveri commented Jan 7, 2021

Ah I remark that one of my comments on N was also pointed out by Henrique. The fix still does not address the 2-level nature of the measurement vs the (occasional) 3-level nature of the system.

You're right, I forgot about this. It also reminds me that we choose the measurement basis in Sequence, which will influence the results here. Do you want me to take a look at this @sebgrijalva ?

@sebgrijalva
Copy link
Contributor Author

I will open a new issue for this three-state measurement. For the moment, the sampling method will only work for two-state measurement.

@lhenriet
Copy link
Member

lhenriet commented Jan 8, 2021

A minor edit that you can maybe put in this pull request. Now that we have the interaction_coeff and rydberg_blockade_radius in device, I think we can safely suppress self._U. What do you think ?

@sebgrijalva
Copy link
Contributor Author

Yes, I forgot about that, thank you.

pulser/simulation.py Outdated Show resolved Hide resolved
pulser/simresults.py Outdated Show resolved Hide resolved
@sebgrijalva
Copy link
Contributor Author

sebgrijalva commented Jan 11, 2021

Thanks, this is looking pretty good. One question: normally one would declare a measurement basis in the sequence (by adding a seq.measure() Timeslot). This basis name is then stored in Sequence._measurement (which so far is private). Then, when calling the method in SimulationResults.sample_final_state, it would read the instruction from the sequence. What do you think about this order of steps?

@HGSilveri
Copy link
Collaborator

HGSilveri commented Jan 11, 2021

I thought about this. The downsides are that it requires making and simulating a new sequence to change the measurement basis and that it might be confusing when you're not sampling, as having a measurement at the end of a Sequence can be equated to a projection of the state vector. When you're interested in the full state vector, I would lean towards a sequence without a measurement.

That being said, perhaps we can make meas_basis a keyword argument meas_basis=None, which when left as None would take the measurement basis from the Sequence. If the Sequence didn't have one, it would throw an error asking for the meas_basis explicitly. What do you think?

@sebgrijalva
Copy link
Contributor Author

That being said, perhaps we can make meas_basis a keyword argument meas_basis=None, which when left as None would take the measurement basis from the Sequence. If the Sequence didn't have one, it would throw an error asking for the meas_basis explicitly. What do you think?

I think this is a good solution, meas_basis as a kwarg gives both options.

@sebgrijalva
Copy link
Contributor Author

So how is this looking now? Can we merge so that the notebooks in (optimal_control and MIS) can be adapted?

@HGSilveri
Copy link
Collaborator

I'll do another review.

Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good overall. After you address these remarks, it should be okay to merge on my end.

pulser/simresults.py Outdated Show resolved Hide resolved
pulser/simresults.py Outdated Show resolved Hide resolved
pulser/simresults.py Outdated Show resolved Hide resolved
pulser/simulation.py Outdated Show resolved Hide resolved
pulser/simulation.py Outdated Show resolved Hide resolved
pulser/simresults.py Outdated Show resolved Hide resolved
Copy link
Member

@lhenriet lhenriet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me.


Keyword Args:
meas_basis (None or str): The basis in which a sampling measurement
is desired.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: needs a tab

Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than that nit, it looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants