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

Allow full gates to be passed into get method of InstructionScheduleMap #5085

Merged
merged 28 commits into from
Nov 1, 2020

Conversation

brosand
Copy link
Contributor

@brosand brosand commented Sep 18, 2020

Summary

In a project on Quantum Optimal Control, I am creating a new class which inherits InstructionScheduleMap ands generates optimal pulses for target unitary operators. As a result, I need to modify the pipeline for the InstructionScheduleMap.get() function. With this change entire gates can be passed in, with their corresponding unitary matrices, and not just the name of the gate.

Details and comments

These changes are very small, just adding in an if statement to check if the instruction passed into InstructionScheduleMap is a gate or a string, and if it is a gate converting it to a string (maintaining the existing pipeline for InstructionScheduleMap). In addition, there is a slight change to lowering.py, to pass a full gate through instead of just a string.

Copy link
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

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

Hi @brosand, this looks mostly good. Could you please just add a test of this functionality to test_instruction_schedule_map.

Copy link
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

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

Does add work with a gate?

@brosand
Copy link
Contributor Author

brosand commented Oct 20, 2020

Does add work with a gate?

No, add is not modified yet.

@taalexander
Copy link
Contributor

Would you mind adding this for this and other related functions so it is consistent? Maybe write a little internal helper function to convert to what you need no matter if you get a string or a gate.

@brosand
Copy link
Contributor Author

brosand commented Oct 20, 2020

Sounds good, I can do that.

Copy link
Contributor

@taalexander taalexander 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, just a couple small suggestions.

@@ -105,7 +108,7 @@ def qubit_instructions(self, qubits: Union[int, Iterable[int]]) -> List[str]:
return list(self._qubit_instructions[_to_tuple(qubits)])
return []

def has(self, instruction: str, qubits: Union[int, Iterable[int]]) -> bool:
def has(self, instruction: Union[str, Gate], qubits: Union[int, Iterable[int]]) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a Gate or a Instruction. Same in other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a question I wanted to discuss with you. Because instructions did not have _to_matrix functionality I only used gates for my project, but since they inherit instruction should I just use instructions everywhere instead of Gate? Is there any reason to limit it to just Gate

Copy link
Contributor

@taalexander taalexander Oct 30, 2020

Choose a reason for hiding this comment

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

I think so because we often input measure which is an instruction. So we should probably use Instruction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switched to instructions. I am still leaving the as test_has_gate and so on for each of the normal tests, since I test with a gate, but I assume that's fine because gates are a type of instruction that is easily accessible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good.

brosand and others added 2 commits October 30, 2020 12:10
Co-authored-by: Thomas Alexander <thomasalexander2718@gmail.com>
taalexander
taalexander previously approved these changes Oct 30, 2020
@taalexander
Copy link
Contributor

@taalexander
Copy link
Contributor

Just a bit of indentation issues 😄

@brosand
Copy link
Contributor Author

brosand commented Oct 30, 2020

Yeah I've been unsure which linter is being used so I have to upload to see if it's right or not, but then I have to wait a while for the azure checks. Should be good now though!

Copy link
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

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

Looks great, as a future reference you should be able to just use make lint to run the linter from the root qiskit directory.

@mergify mergify bot merged commit 074a867 into Qiskit:master Nov 1, 2020
@1ucian0 1ucian0 added the mod: pulse Related to the Pulse module label Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: pulse Related to the Pulse module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants