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

Add simulate method to Circuit #456

Closed
wants to merge 5 commits into from
Closed

Add simulate method to Circuit #456

wants to merge 5 commits into from

Conversation

Strilanc
Copy link
Contributor

Currently implemented by delegating to the XmonSimulator, but this is a hidden implementation detail.

@Strilanc Strilanc requested a review from dabacon June 12, 2018 22:38
@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Jun 12, 2018
Copy link
Collaborator

@dabacon dabacon left a comment

Choose a reason for hiding this comment

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

This really seems to make cohesion in Circuit much worse (Circuit methods are about building and accessing circuits). I think we should make classes cohesive. Along these lines I actually think we maybe made a mistake in having to_unitary_matrix on the Circuit class. I also think that this will not scale well as we add more simulators or simulators with different features, and by keeping the soup of simulators separate it won't leak over into circuit

@Strilanc
Copy link
Contributor Author

I think we have two types of circuit methods: manipulation methods, and inspection methods. Manipulation methods include insert, clear, and append. A natural addition to this group might be "apply optimization". Inspection methods include gate querying, slicing, and printing. "Tell me about the behavior of this circuit" seems like a natural fit here, to me.

I also think there is great value in not having to make a bunch of objects to do common tasks. The fact that I have to make an XmonSimulator object (giving it no arguments) then call simulate on it seems superfluous compared to just having a raw simulate method, and if we're going to have a raw simulate method it might as well be a member of Circuit.

Having the method be a member is also an example of what I'm going to call "autocomplete-based design". It makes it easier for the user to answer the question "how do I simulate a circuit?", because the natural instinct when you have a "how do Y to X" is to dot off of X and see what your IDE says is available to call.

So I would say I'm not worried about the cohesion. What I do worry about with this change is the effects on coupling. This is the first instance of a circular dependency workaround.

@dabacon
Copy link
Collaborator

dabacon commented Jun 13, 2018 via email

@Strilanc
Copy link
Contributor Author

Based on the group discussion, we're going to do something different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants