-
Notifications
You must be signed in to change notification settings - Fork 134
Update CompositeAnalysis initialization #633
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 from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,11 +13,11 @@ | |||||||
| Composite Experiment Analysis class. | ||||||||
| """ | ||||||||
|
|
||||||||
| from typing import List, Dict | ||||||||
|
|
||||||||
| from typing import List, Dict, Union | ||||||||
| import numpy as np | ||||||||
| from qiskit.result import marginal_counts | ||||||||
| from qiskit_experiments.framework import BaseAnalysis, ExperimentData | ||||||||
| from qiskit_experiments.exceptions import AnalysisError | ||||||||
|
|
||||||||
|
|
||||||||
| class CompositeAnalysis(BaseAnalysis): | ||||||||
|
|
@@ -45,31 +45,69 @@ class CompositeAnalysis(BaseAnalysis): | |||||||
| reconstructed from the parent composite experiment data. | ||||||||
| """ | ||||||||
|
|
||||||||
| def __init__(self, analyses: List[BaseAnalysis]): | ||||||||
| """Initialize a composite analysis class. | ||||||||
|
|
||||||||
| Args: | ||||||||
| analyses: a list of component experiment analysis objects. | ||||||||
| """ | ||||||||
| super().__init__() | ||||||||
| self._analyses = analyses | ||||||||
|
|
||||||||
| def component_analysis(self, index=None) -> Union[BaseAnalysis, List[BaseAnalysis]]: | ||||||||
| """Return the component experiment Analysis object""" | ||||||||
| if index is None: | ||||||||
| return self._analyses | ||||||||
| return self._analyses[index] | ||||||||
|
|
||||||||
| def _run_analysis(self, experiment_data: ExperimentData): | ||||||||
| # Return list of experiment data containers for each component experiment | ||||||||
| # containing the marginalied data from the composite experiment | ||||||||
| component_exp_data = self._component_experiment_data(experiment_data) | ||||||||
|
|
||||||||
| # Run the component analysis on each component data | ||||||||
| for sub_exp_data, sub_analysis in zip(component_exp_data, self._analyses): | ||||||||
| # Since copy for replace result is handled at the parent level | ||||||||
| # we always run with replace result on component analysis | ||||||||
| sub_analysis.run(sub_exp_data, replace_results=True) | ||||||||
|
|
||||||||
| # Wait for all component analysis to finish before returning | ||||||||
| # the parent experiment analysis results | ||||||||
| for sub_exp_data in component_exp_data: | ||||||||
| sub_exp_data.block_for_results() | ||||||||
|
|
||||||||
| return [], [] | ||||||||
|
|
||||||||
| def _component_experiment_data(self, experiment_data: ExperimentData) -> List[ExperimentData]: | ||||||||
| """Return a list of component child experiment data""" | ||||||||
| # Initialize component data for updating and get the experiment IDs for | ||||||||
| # the component child experiments in case there are other child experiments | ||||||||
| # in the experiment data | ||||||||
| component_ids = self._initialize_components(experiment_data) | ||||||||
| if len(component_ids) != len(self._analyses): | ||||||||
| raise AnalysisError( | ||||||||
| "Number of experiment components does not match number of" | ||||||||
| " component analysis classes" | ||||||||
| ) | ||||||||
|
|
||||||||
| # Extract job metadata for the component experiments so it can be added | ||||||||
| # to the child experiment data incase it is required by the child experiments | ||||||||
| # analysis classes | ||||||||
| composite_exp = experiment_data.experiment | ||||||||
| component_exps = composite_exp.component_experiment() | ||||||||
| component_metadata = experiment_data.metadata.get( | ||||||||
| "component_metadata", [{}] * composite_exp.num_experiments | ||||||||
| "component_metadata", [{}] * len(component_ids) | ||||||||
| ) | ||||||||
|
|
||||||||
| # Initialize component data for updating and get the experiment IDs for | ||||||||
| # the component child experiments in case there are other child experiments | ||||||||
| # in the experiment data | ||||||||
| component_ids = self._initialize_components(composite_exp, experiment_data) | ||||||||
|
|
||||||||
| # Compute marginalize data for each component experiment | ||||||||
| marginalized_data = self._marginalize_data(experiment_data.data()) | ||||||||
| marginalized_data = self._component_data(experiment_data.data()) | ||||||||
|
|
||||||||
| # Add the marginalized component data and component job metadata | ||||||||
| # to each component child experiment. Note that this will clear | ||||||||
| # any currently stored data in the experiment. Since copying of | ||||||||
| # child data is handled by the `replace_results` kwarg of the | ||||||||
| # parent container it is safe to always clear and replace the | ||||||||
| # results of child containers in this step | ||||||||
| for i, (sub_data, sub_exp) in enumerate(zip(marginalized_data, component_exps)): | ||||||||
| component_data = [] | ||||||||
| for i, sub_data in enumerate(marginalized_data): | ||||||||
| sub_exp_data = experiment_data.child_data(component_ids[i]) | ||||||||
|
|
||||||||
| # Clear any previously stored data and add marginalized data | ||||||||
|
|
@@ -78,44 +116,11 @@ def _run_analysis(self, experiment_data: ExperimentData): | |||||||
|
|
||||||||
| # Add component job metadata | ||||||||
| sub_exp_data.metadata.update(component_metadata[i]) | ||||||||
| component_data.append(sub_exp_data) | ||||||||
|
|
||||||||
| # Run analysis | ||||||||
| # Since copy for replace result is handled at the parent level | ||||||||
| # we always run with replace result on component analysis | ||||||||
| sub_exp.analysis.run(sub_exp_data, replace_results=True) | ||||||||
|
|
||||||||
| # Wait for all component analysis to finish before returning | ||||||||
| # the parent experiment analysis results | ||||||||
| for comp_id in component_ids: | ||||||||
| experiment_data.child_data(comp_id).block_for_results() | ||||||||
|
|
||||||||
| return [], [] | ||||||||
|
|
||||||||
| def _initialize_components(self, experiment, experiment_data): | ||||||||
| """Initialize child data components and return list of child experiment IDs""" | ||||||||
| # Check if component child experiment data containers have already | ||||||||
| # been created. If so the list of indices for their positions in the | ||||||||
| # ordered dict should exist. Index is used to extract the experiment | ||||||||
| # IDs for each child experiment which can change when re-running analysis | ||||||||
| # if replace_results=False, so that we update the correct child data | ||||||||
| # for each component experiment | ||||||||
| component_index = experiment_data.metadata.get("component_child_index", []) | ||||||||
| if not component_index: | ||||||||
| # If the experiment Construct component data and update indices | ||||||||
| start_index = len(experiment_data.child_data()) | ||||||||
| component_index = [] | ||||||||
| for i, sub_exp in enumerate(experiment.component_experiment()): | ||||||||
| sub_data = sub_exp._initialize_experiment_data() | ||||||||
| experiment_data.add_child_data(sub_data) | ||||||||
| component_index.append(start_index + i) | ||||||||
| experiment_data.metadata["component_child_index"] = component_index | ||||||||
|
|
||||||||
| # Child components exist so we can get their ID for accessing them | ||||||||
| child_ids = experiment_data._child_data.keys() | ||||||||
| component_ids = [child_ids[idx] for idx in component_index] | ||||||||
| return component_ids | ||||||||
| return component_data | ||||||||
|
|
||||||||
| def _marginalize_data(self, composite_data: List[Dict]) -> List[Dict]: | ||||||||
| def _component_data(self, composite_data: List[Dict]) -> List[List[Dict]]: | ||||||||
| """Return marginalized data for component experiments""" | ||||||||
| # Marginalize data | ||||||||
| marginalized_data = {} | ||||||||
|
|
@@ -148,3 +153,33 @@ def _marginalize_data(self, composite_data: List[Dict]) -> List[Dict]: | |||||||
|
|
||||||||
| # Sort by index | ||||||||
| return [marginalized_data[i] for i in sorted(marginalized_data.keys())] | ||||||||
|
|
||||||||
| def _initialize_components(self, experiment_data: ExperimentData) -> List[str]: | ||||||||
| """Initialize child data components and return list of child experiment IDs""" | ||||||||
| # Check if component child experiment data containers have already | ||||||||
| # been created. If so the list of indices for their positions in the | ||||||||
| # ordered dict should exist. Index is used to extract the experiment | ||||||||
| # IDs for each child experiment which can change when re-running analysis | ||||||||
| # if replace_results=False, so that we update the correct child data | ||||||||
| # for each component experiment | ||||||||
| component_index = experiment_data.metadata.get("component_child_index", []) | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a really property of the experiment data, not the analysis, and is required if you re-run analysis to update existing results for containers that could contain other child data not related to the composite experiment (if someone manually added child data for some reason) |
||||||||
| if not component_index: | ||||||||
| experiment = experiment_data.experiment | ||||||||
| if experiment is None: | ||||||||
| raise AnalysisError( | ||||||||
| "Cannot run composite analysis on an experiment data without either " | ||||||||
| "a composite experiment, or composite experiment metadata." | ||||||||
| ) | ||||||||
| # If the experiment Construct component data and update indices | ||||||||
| start_index = len(experiment_data.child_data()) | ||||||||
| component_index = [] | ||||||||
| for i, sub_exp in enumerate(experiment.component_experiment()): | ||||||||
| sub_data = sub_exp._initialize_experiment_data() | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be called before analysis is called? For example we can insert hook method which might be called before job execution. Then we can completely decouple experiment from the experiment data. I think initialization of container should be a part of composite experiment rather than analysis.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @nkanazawa1989
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done by experiment.run. This block here is only called if you try and run analysis on a data that was not initialized via an experiment.run, such as if you loaded job ids and manually added composite experiment jobs and then re-run analysis.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I should point out this block code isn't something added in this PR, I just moved it, and added an extra warning. |
||||||||
| experiment_data.add_child_data(sub_data) | ||||||||
| component_index.append(start_index + i) | ||||||||
| experiment_data.metadata["component_child_index"] = component_index | ||||||||
|
|
||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably
Suggested change
likely this is singleton and this is evidence of user might be doing something unexpected.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The child experiment data should already have been initialized |
||||||||
| # Child components exist so we can get their ID for accessing them | ||||||||
| child_ids = experiment_data._child_data.keys() | ||||||||
| component_ids = [child_ids[idx] for idx in component_index] | ||||||||
| return component_ids | ||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,11 +13,12 @@ | |||||||||||
| Composite Experiment abstract base class. | ||||||||||||
| """ | ||||||||||||
|
|
||||||||||||
| from typing import List, Sequence, Optional | ||||||||||||
| from typing import List, Sequence, Optional, Union | ||||||||||||
| from abc import abstractmethod | ||||||||||||
| import warnings | ||||||||||||
| from qiskit.providers.backend import Backend | ||||||||||||
| from qiskit_experiments.framework import BaseExperiment, ExperimentData | ||||||||||||
| from qiskit_experiments.framework.base_analysis import BaseAnalysis | ||||||||||||
| from .composite_analysis import CompositeAnalysis | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
|
|
@@ -41,9 +42,10 @@ def __init__( | |||||||||||
| """ | ||||||||||||
| self._experiments = experiments | ||||||||||||
| self._num_experiments = len(experiments) | ||||||||||||
| analysis = CompositeAnalysis([exp.analysis for exp in self._experiments]) | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @yaelbh 's concern. Probably this will fix the problem.
Suggested change
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After that you won't be able anymore to run analysis on a sub-experiment, independently from it being part of a composite experiment. |
||||||||||||
| super().__init__( | ||||||||||||
| qubits, | ||||||||||||
| analysis=CompositeAnalysis(), | ||||||||||||
| analysis=analysis, | ||||||||||||
| backend=backend, | ||||||||||||
| experiment_type=experiment_type, | ||||||||||||
| ) | ||||||||||||
|
|
@@ -57,8 +59,9 @@ def num_experiments(self): | |||||||||||
| """Return the number of sub experiments""" | ||||||||||||
| return self._num_experiments | ||||||||||||
|
|
||||||||||||
| def component_experiment(self, index=None): | ||||||||||||
| def component_experiment(self, index=None) -> Union[BaseExperiment, List[BaseExperiment]]: | ||||||||||||
| """Return the component Experiment object. | ||||||||||||
|
|
||||||||||||
| Args: | ||||||||||||
| index (int): Experiment index, or ``None`` if all experiments are to be returned. | ||||||||||||
| Returns: | ||||||||||||
|
|
@@ -68,9 +71,16 @@ def component_experiment(self, index=None): | |||||||||||
| return self._experiments | ||||||||||||
| return self._experiments[index] | ||||||||||||
|
|
||||||||||||
| def component_analysis(self, index): | ||||||||||||
| def component_analysis(self, index=None) -> Union[BaseAnalysis, List[BaseAnalysis]]: | ||||||||||||
| """Return the component experiment Analysis object""" | ||||||||||||
| return self.component_experiment(index).analysis() | ||||||||||||
| warnings.warn( | ||||||||||||
| "The `component_analysis` method is deprecated as of " | ||||||||||||
| "qiskit-experiments 0.3.0 and will be removed in the 0.4.0 release." | ||||||||||||
| " Use `analysis.component_analysis` instead.", | ||||||||||||
| DeprecationWarning, | ||||||||||||
| stacklevel=2, | ||||||||||||
| ) | ||||||||||||
| return self.analysis.component_analysis(index) | ||||||||||||
|
|
||||||||||||
| def copy(self) -> "BaseExperiment": | ||||||||||||
| """Return a copy of the experiment""" | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| --- | ||
| features: | ||
| - | | ||
| Adds :meth:`.CompositeAnalysis.component_analysis` method for accessing | ||
| a component analysis class object from a composite analysis object. | ||
| upgrade: | ||
| - | | ||
| :class:`.CompositeAnalysis` initialization is changed to require a list of | ||
| :class:`.BaseAnalysis` objects so that these are stored in the class, rather | ||
| than being accessed later via a composite experiment. This initialization is | ||
| handled automatically by :class:`.ParallelExperiment` and | ||
| :class:`.BatchExperiment` composite experiments. | ||
| deprecations: | ||
| - | | ||
| The :meth:`.CompositeExperiment.component_analysis` method has been | ||
| deprecated. Component analysis classes should now be directly accessed | ||
| from a :meth:`.CompositeAnalysis` object using the | ||
| :meth:.`CompositeAnalysis.component_analysis` method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write unittest to check this? I feel the mapping of experiment and analysis is no longer obvious as before and we need to guarantee new mechanism works correctly. Something like this could be worth testing (or more complicated, such as nested parallel into batch and vise versa).
This should success since this line cannot be modified by users without hack, but someone may update the logic in future.
https://github.com/Qiskit/qiskit-experiments/pull/633/files#diff-dfb1550e8c0375a7cf9c4a2e0f5ce96f30b17125796008307d1e9793af7444a7R45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are already a lot of composite experiment tests but i think they all use the same component experiment, it would be good to add some tests that mix different experiments like you suggest