Update composite experiment and analysis initialization #720
Conversation
nkanazawa1989
left a comment
nkanazawa1989
left a comment
There was a problem hiding this comment.
I almost copied my comments from #721 but did more careful review. Overall instantiating the child experiment data at the beginning of analysis routine looks nice. Seems like this makes the analysis more robust to the complicated situation, such as nested composite experiment.
| # Store the indices of the added child data in metadata | ||
| experiment_data.metadata["component_child_index"] = component_index | ||
|
|
||
| def _initialize_component_data(self, experiment_data: ExperimentData) -> List[ExperimentData]: |
There was a problem hiding this comment.
Having initialize_component_data and initialize_child_data are really confusing for developers. As far as I can understand, there is no physical component_data member in this class (indeed this is deprecated attribute), and what this method is doing looks like initializing the data container for the experiment data of child experiment. What about renaming this to _create_empty_containers? Probably removing this method and just merging the logic here to _initialize_child_data is also fine.
There was a problem hiding this comment.
_initialize_component_data creates the empty component ExperimentData objects for analysis, but it doesn't save them as child data. This needs to be separate to _initialize_child_data for the follow up PR for combining results where you want to generate these, analyse them, but not necessarily save them as child experiments.
| # and auto save from the parent experiment data | ||
| component_expdata = [] | ||
| for i, _ in enumerate(self._analyses): | ||
| subdata = ExperimentData(backend=experiment_data.backend) |
There was a problem hiding this comment.
How does this work if composite analysis is nested? Probably we need instance check of analysis and do recursive call of initialize method when composite analysis subclass is found.
There was a problem hiding this comment.
Arbitrary nesting is fine. The point of doing it this way is that the analysis of each component is independent, so when the component analysis is run on the sub-experiment, if it is itself a composite experiment it will then go through this same initialization and for that component and its children.
| self._experiments = experiments | ||
| self._num_experiments = len(experiments) | ||
| analysis = CompositeAnalysis([exp.analysis for exp in self._experiments]) | ||
| if analysis is None: |
There was a problem hiding this comment.
We need instance check. If user provides custom analysis that directly inherits from the base analysis, the mechanism to instantiate child container is missing there.
| # added to the component experiment data containers | ||
| metadata = experiment_data.metadata | ||
| num_components = len(self._analyses) | ||
| experiment_types = metadata.get("component_types", [None] * num_components) |
There was a problem hiding this comment.
Can we retrieve old composite analysis class from the experiment db?
There was a problem hiding this comment.
Old data can still be loaded and analyzed, since old data will have the child_data already initialized (and have hence have "component_child_index" metadata field) this bit will be skipped.
| # 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) | ||
| component_expdata = self._component_experiment_data(experiment_data) |
There was a problem hiding this comment.
Should _component_experiment_data be renamed now because component_experiment_data attribute has been already renamed in the 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) | ||
| component_expdata = self._component_experiment_data(experiment_data) |
There was a problem hiding this comment.
Should _component_experiment_data be renamed now because component_experiment_data attribute has been already renamed in the ExperimentData?
There was a problem hiding this comment.
This really is the component experiments experiment data though, so i think it's the appropriate name in this class
Also add to Batch and Parallel Experiment. This is intended to make it easier to attach a custom analysis class to subclasses of these experiments.
chriseclectic
force-pushed
the
comp-init
branch
from
cea62aa to
3840153
Compare
| start_index = len(experiment_data.child_data()) | ||
| for i, subdata in enumerate(child_components): | ||
| experiment_data.add_child_data(subdata) | ||
| component_index.append(start_index + i) |
There was a problem hiding this comment.
This is because we can add new child data on top of previous data?
…nity#720) * Update initialization of CompositeAnalysis * Add analysis kwarg to CompositeExpereiments Also add to Batch and Parallel Experiment. This is intended to make it easier to attach a custom analysis class to subclasses of these experiments.
chriseclectic
added
Changelog: New Feature
…nity#720) * Update initialization of CompositeAnalysis * Add analysis kwarg to CompositeExpereiments Also add to Batch and Parallel Experiment. This is intended to make it easier to attach a custom analysis class to subclasses of these experiments.
2 participants
Summary
analysiskwarg toCompositeExperiment,BatchExperimentandParallelExperimentto allow a user to supply a customCompositeAnalysisinstance. This may be used by subclass experiments.Details and comments
When initializing the child experiment data during analysis specific properties of the parent data will be copied to the child data including tags, share level, and auto save, so that to an end user the behavior of setting this attributes is unchanged by this PR.