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

🛩️ 🏁 Removes the template specification process and cleans up template class 🐰 #193

Merged
merged 14 commits into from
Mar 26, 2024

Conversation

rryoung98
Copy link
Contributor

Closes #186

@smburdick
Copy link
Contributor

Thanks @rryoung98 . This is exactly what I had in mind when we last spoke.

Per this morning's discussion, we'll want to be able to produce "plaquette footprints," which aren't the same as plaquette templates per se, hence why we needed to change the frontend to reflect that.

I think it would be nice to have the ability to specify the plaquette footprint using the infrastructure that exists within the Template class, which we can then rename.

To summarize, the workflow would look like:

Unit cell -> specify footprint (using click-and-drag) -> specify circuit on plaquette, based on the footprint.

Additionally, once a footprint has been specified, we could be able to choose it from the library. Task #189 will provide the infrastructure for the library, which can include footprints, completed circuits, and scalable templates.

@afowler Do you have any more questions/commentary here? In particular, how would we define a template, now that we have footprints defined?

@afowler
Copy link

afowler commented Mar 13, 2024 via email

Copy link
Contributor

@smburdick smburdick left a comment

Choose a reason for hiding this comment

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

Looks good overall, but making some suggestions based on this morning's meeting.

frontend/src/plaquettes/Template.js Show resolved Hide resolved
frontend/src/plaquettes/Template.js Outdated Show resolved Hide resolved
frontend/src/plaquettes/Template.js Show resolved Hide resolved
frontend/src/plaquettes/Template.js Outdated Show resolved Hide resolved
@@ -263,13 +128,9 @@ export default class Template {
}
// Render the plaquette
const plaquette = new Plaquette(this.selectedQubits, this.workspace, this.app);
// Remove seleected qubits from the template qubits, so they can be
Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I don't think a Footprint should remember the specific set of qubits (in the viewport) used to create it, but should remember a cell of qubit positions (just like the unit cell) for re-creating the footprint anywhere on the viewport. Can we add that to this class?

Copy link
Contributor Author

@rryoung98 rryoung98 Mar 19, 2024

Choose a reason for hiding this comment

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

yes, would you mind clarifying how we should go about the positioning?

Copy link
Contributor

@smburdick smburdick Mar 19, 2024

Choose a reason for hiding this comment

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

When it is placed, that the absolute position obviously must known to the workspace to draw its position. When it is stored (say, in redux), we only need to store the relative positions. There's a 1-1 correspondence, and storing relative positions helps us keep things general, such as when a new plaquette is added from the library, or an existing workspace created by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay makes sense, w.r.t. relative position could you point me towards any existing code that lays out the foundation?

frontend/src/plaquettes/Template.js Outdated Show resolved Hide resolved
frontend/src/plaquettes/Template.js Show resolved Hide resolved
@smburdick
Copy link
Contributor

For the moment they should be only one footprint for everything. For the moment we don't plan to combine plaquettes with different footprints in a single circuit.

@afowler OK, we can leave footprint specification as-is then. I'll add plaquette footprint saving/specification to the next milestone.

@afowler
Copy link

afowler commented Mar 13, 2024 via email

@smburdick
Copy link
Contributor

smburdick commented Mar 13, 2024

could be a subset, for example a triangle that attaches to the bottom, top, right, or left of the footprint.

To be clear, the entire surface code that the user is building has only one footprint?

@afowler That makes sense, but would the triangular plaquette still be something we want in the plaquette library? What would the use case for creating one be? I think the purpose of the library would be to have different circuits.

@afowler
Copy link

afowler commented Mar 13, 2024 via email

@smburdick
Copy link
Contributor

@rryoung98 Is this ready for re-review?

@rryoung98
Copy link
Contributor Author

@smburdick i haven't done the relative positioning since the frontend version 2, if i recall might already have the relative positioning notion. Otherwise, it's all set for re-review

@smburdick
Copy link
Contributor

@smburdick i haven't done the relative positioning since the frontend version 2, if i recall might already have the relative positioning notion. Otherwise, it's all set for re-review

Thanks, and in the future, be sure to request that from the reviewer when ready.

Copy link
Contributor

@smburdick smburdick left a comment

Choose a reason for hiding this comment

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

Minor comments.

frontend/src/plaquettes/Footprint.js Outdated Show resolved Hide resolved
frontend/src/plaquettes/Footprint.js Outdated Show resolved Hide resolved
@rryoung98
Copy link
Contributor Author

@smburdick please re-review

Copy link
Contributor

@smburdick smburdick left a comment

Choose a reason for hiding this comment

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

image

I think we should remove this label as a button, since I don't have to actually click it to make a new footprint -- I can still make plaquettes by clicking on qubits, and clicking "create plaquette". So, instead of having the button, we can simply add the text to the topbar.

Looks good otherwise, you can make the changes here, or create a new issue to address that.

@rryoung98
Copy link
Contributor Author

image I think we should remove this label as a button, since I don't have to actually click it to make a new footprint -- I can still make plaquettes by clicking on qubits, and clicking "create plaquette". So, instead of having the button, we can simply add the text to the topbar.

Looks good otherwise, you can make the changes here, or create a new issue to address that.

Done

@rryoung98 rryoung98 merged commit 8243350 into main Mar 26, 2024
5 checks passed
@rryoung98 rryoung98 deleted the QSP-fix-template_border-2024.03.13 branch March 26, 2024 16:45
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.

Change template specification to footprint specification
3 participants