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

Changes to (_internal_)create_program() #327

Merged

Conversation

lumip
Copy link
Contributor

@lumip lumip commented Aug 2, 2018

Implementations of new signature PT._internal_create_program for RepetitionPT, SequencePT, ForLoopPT, AtomicPT (and thus all subclasses of that) + tests.

MappingPT is the only one missing now.
After that is done, #287 should be finished.

Copy link
Member

@terrorfisch terrorfisch 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 beside some stuff that might be to slow in some cases. But we can tackle this if it becomes a problem.

@@ -118,7 +118,7 @@ def create_program(self, *,
if parameters is None:
parameters = dict()
if measurement_mapping is None:
measurement_mapping = dict()
measurement_mapping = dict() # todo (2018-08-02): should not be empty but an identity mapping; requires PT to be a MeasurementDefiner (?)
Copy link
Member

Choose a reason for hiding this comment

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

measurement_names is an abstract property of PulseTemplate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh... done :D

measurement_parameters = {parameter_name: parameters[parameter_name].get_value()
for parameter_name in self.measurement_parameters}
except KeyError as e:
raise ParameterNotProvidedException(str(e)) from e
Copy link
Member

Choose a reason for hiding this comment

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

Why do you create an exta dictionary for the measurement parameters? They should be a subset of parameter_names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be but currently aren't according to #230

measurements = self.get_measurement_windows(measurement_parameters, measurement_mapping)
if self.duration.evaluate_numeric(**duration_parameters) > 0:
Copy link
Member

Choose a reason for hiding this comment

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

There should be a better way. But this is an optimization for the future.

… additional test for ForLoopPT._internal_create_program()
…easurement_mapping by replacing them with an identity mapping for all measurement names
@terrorfisch terrorfisch merged commit 34bffaa into issues/287_pt_create_program Aug 2, 2018
@lumip lumip deleted the issues/287_pt_create_program_change branch August 8, 2018 07:26
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.

None yet

2 participants