Add child data support to ExperimentData

[chriseclectic]

Summary

This PR build on #364 to integrate the parent-child experiment data relationships supported in the experiment DB into the ExperimentData class. The main motivation for this is to remove having multiple subclasses of ExperimentData so that all experiments use the same data class, and this is the only user facing data class for interacting with experiment results and result database.

Details and comments

A couple of other API changes included in this PR are:

• Remove CompositeExperimentData class (functionality is now in ExperimentData)
• remove __experiment_data__ attribute from BaseExperiment
• Remove experiment_data kwarg from BaseExperiment.run.
• Add add_child_data methods to ExperimentData for adding a child experiment data container
• Add child_data method to ExperimentData for accessing child data containers (this is same functionality of previous component_experiment_data from CompositeExperimentData). The component naming was changed to child to avoid confusion with the experiment components used for defining experiments and analysis results.

[yaelbh]

Partition to at least three smaller PRs. For example (similar to what you wrote):

• Removal of CompositeExperimentData.
• Refactor of AnalysisResult.run and creation of AnalysisResult.run_analysis.
• A new update parameter in AnalysisResult.run and removal of experiment_data from the list of parameters of BaseExperiment.run.
• blcok_for_jobs refactor.
• Removal of __experiment_data__ from BaseExperiment.

It appears that the existing tests are not enough for testing these changes. I think I found a few bugs, if indeed then this means that effort has to be done to increase test coverage and better verify the changes.

#209 is relevant in this context. I started to work on #209 for T1. Think how you can test stuff that's related to saving and loading without waiting for #209 to be done.

[yaelbh]

Previously the data was marginalized in CompositeExperimentData, now it happens during analysis. But this new location is incorrect. Data can change without performing analysis.

In addition, if we rerun analysis on data that wasn't changed since the last time, we rerun _marginalize_data, which is not needed and can be expensive.

And: what if we analyze only a child, without analyzing the parent, while the data was changed once: the child analysis will be carried out on the old data, without being updated.

[chriseclectic]

@yaelbh Split out two separate PR for changes to BaseExperiment.run (#463), and BaseAnalysis.run (#464) which should be merged before this.

[chriseclectic]

@yaelbh I've reworked the composite experiment so that all the handling of child experiment data and marginalization is in the analysis class. Unfortunately this still depends on the original composite experiment (but no longer the child data already being initialized). Eventually I'd like to remove this dependency but that will have to wait till after refactoring how analysis class and metadata is handled in the experiment framework.

[yaelbh]

We face the following constraints:

1. Sub-experiments data must be updated whenever data is added, for the reasons that I outlined in Add child data support to ExperimentData #451 (comment).
2. You want to be able to analyze data that was constructed without calling add_data.

I see two solutions. I’m OK with both, although I'm very strongly inclined towards the first one. They both require to externalize the method that computes sub-experiments data from the main data.

1. Maintain an invariant that sub-experiments data must all the time be consistent with the main data. If you construct your data using add_data, then add_data guarantees for you that the invariant is respected. Otherwise, if you construct your data in any other way, then it’s your responsibility to maintain the invariant (you’re invited to use a method that we provide for updating sub-experiments data).
2. Sub-experiments data is computed in both locations: in add_data, and also during analysis. We can in addition (not mandatory) introduce a Boolean flag that signals whether the sub-experiments are up-to-date.

As a side note, I think _marginalize_data should work directly on the experiment data (and not on experiment_data.data()). Metadata of experiment_data.data is very similar to experiment_data.metadata and can probably be removed. Instead of working with composite_index, composite_clbits etc., work with the information that's stored in the experiment data and its descendants. But anyway, this is only a side note, not for this PR.

[chriseclectic]

In my view in general a child experiment data is independent of its parent. It's just connected via an ID and a user can create or remove that connection if they want to (I'm not sure how removing works with DB, something to try), so adding data to either one doesn't intrinsically have anything to do with the other. We could make the add_child_data method internal (_add_child_data) so for now a user won't create this connection arbitrarily though.

CompositeExperiments are a specific use case (and for the moment the only use case), and in this case the child-data is a by-product of analysis of the main experiment data. That is why I think marginalization and creation of the marginalized child data should be entirely handled by the analysis of that experiment. In this case if you modify a child experiment, but then re-run the main analysis, you will lose your changes since it will be overridden by the regeneration of data and results from the main analysis.

The circuit level metadata can't be moved to the main experiment, since then there is no way to combine experiments with different number of circuits, because each circuit may only contain some subset of the component experiments, not all of them. Each circuit could also contain different numbers of classical bits so the bits being marginalized to for each sub experiment can change on a circuit to circuit basis.

[nkanazawa1989]

I like the new approach this PR implements. But the logic in CompositeAnalysis._run_analysis is complicated so more documentation is necessary for friendliness for developers. Once one finds ExperimentData.child_data is not automatically created, it's easy to read the logic but otherwise this looks super complicated.

[nkanazawa1989]

Suggested change

	component_exps = composite_exp.component_experiment()
	component_exps = composite_exp.component_experiments()

?

[chriseclectic]

This is the current name of the method in the CompositeExperiment class

[nkanazawa1989]

This is suggestion from @wshanks #460 (comment)

[chriseclectic]

It could be changed, would just be an API change.

[wshanks]

component_experiment() could be kept if desired. It seems to me like making component_experiments a property that gives back _experiments would be best. My point in that comment was that the API of a function with a singular name that takes an index and returns one value and also takes no input and gives back all values is confusing, so I would just make a separate plural API point that gives back all the values and has a plural name for that case.

[chriseclectic]

I would also be fine with just having the plural version and allowing it to take None, int or slice to return all, single, or subset like we do with analysis_resultsand child_data, but that can be done in separate PR

[nkanazawa1989]

Suggested change

	"experiment_options": copy.copy(sub_exp.experiment_options.__dict__),
	"experiment_options": sub_exp.experiment_options.__dict__.copy(),

probably this is simpler?

[nkanazawa1989]

Overriding parent_id of the experiment_data without changing its id doesn't break reference structure in the remote db? (If it is retrieved from db)

[chriseclectic]

Good question! Need to check what happens if you change the parent ID of data already saved to DB. I expected it was just a field that could be changed, but havent tested myself.

[nkanazawa1989]

Suggested change

	sub_exp_data._metadata["job_metadata"] = [component_metadata[i]]
	sub_exp_data._metadata["job_metadata"].update(component_metadata[i])

component_metadata can be empty (I guess this is the case when manually adding retrieved data). This should not override original data.

[chriseclectic]

This is intended to override the data. Since these containers are dynamically created and updated by analysis there is no reason to keep any previous job data (There is actually no need to have this as a list in base experiment since #463 , since you can't re-run in an existing container).

[yaelbh]

I'm trying to review the code but I'm getting lost, especially in composite_analysis.py. Can you please add, somewhere in the code, an extensive documentation of the data structures of: experiment_data, experiment_data.data, experiment_data.metadata, experiment_data.data.metadata. What do you expect in each? What are the keys that are expected to exist in the different dictionaries, what do their values mean and what are their data structures, in which cases would these keys exist? What are the attributes that are expected to exist in different class objects, what do their values mean and what are their data structures, in which cases would these attributes exist? What about circuit level data, which information goes there, when, and why? How is the information propagated, what's its flow - for example, I think circuit data is copied to experiment_data.data in DbExperimentData.add_data - is this correct? Any other things are copied from one place to another: if yes, then what and when, can the original or the copy change after the copy, and what happens then?

Please illustrate a few common use cases. When and where can data be added or modified, for parents and children, when and where can analysis be carried on, for parents and children? What are the consequences of such operations on the data structures, for the case where replace_results is True, and for the case where it's False? When and where are the sub-experiment data created?

It all feels delicate and fragile. I wouldn't know to maintain it and I can't review it. I hope that good documentation will be enough to solve this, and that we won't have to further change the code for an improved design. But I can't tell for now.

[yaelbh]

I don't understand these lines. What does the existence of component_job_metadata in the metadata mean, and why, if it exists, are you taking the last of experiment_data.metadata["component_job_metadata"][-1] ? Please write inline documentation to explain.

[nkanazawa1989]

If I understand correctly the component_job_metadata is another set of metadata to initialize child experiment data. This is generated per job run so this means the last set of children. The metadata also contains job_metadata but this represents configurations of a full set of composite experiment (i.e. parent) so this doesn't help analysis. I don't think job_metadata is really necessary for composite experiment (more specifically, only run options is necessary to reconstruct experiment because other configs are usually unique to children).

[nkanazawa1989]

The important point we need to understand here is children is not generated by job execution, but this is generated on the fly by analysis. This is because experiment data child is not limited only to the composite, but also, for example, we can create pseudo RDB of experiment data. i.e.

my_all_result_for_writing_prx_paper = ExperimentData()
my_all_result_for_writing_prx_paper.add_child_data(my_data_of_calibration)
my_all_result_for_writing_prx_paper.add_child_data(my_data_of_rb)
my_all_result_for_writing_prx_paper.add_child_data(my_data_of_quantum_volume)
etc...

then we can write my_all_result_for_writing_prx_paper id in paper for reproducibility.

[chriseclectic]

This is to deal with how regular ExperimentData stores job metadata, which is used by some analysis (curve analysis in particular i think). If its not there certain experiments analysis wont run (note I think this should be changed, but it's beyond the scope of this PR).

Since each component experiment might need this for its respective analysis to work this is just storing the list of these component job metadata in the parent experiment so it can be added later along with the marginalized data. Previously this was all done when calling run which initialized all the child containers, but this is just moving it to analysis.

The [-1] was because this structure is a list for each run job, and you want the most recent one (same as job_metadata). Since #463 this isn't really necessary since you can't re-run more experiment jobs into the same container.

[chriseclectic]

@yaelbh The main use cases haven't changed. You can run parallel and batch experiments the same as you could before. The only difference is the component experiments data is cleared and as part of analysis.run instead of experiment.run since this is significantly more robust than how it was handled before. It was more delicate and fragile before these changes.

Eg you can use it like:

par_exp = ParallelExperiment([StateTomography(XGate(), qubits=[i]) for i in range(3)])
par_data = par_exp.run(sim).block_for_results()
for sub_data in par_data.child_data():
    print(sub_data.experiment_type)
    for result in sub_data.analysis_results():
        print(result)

Then if you want to try and break things you can add some dummy child data from another experiment:

other_data = StandardRB(qubits, lengths).run(backend)
par_data.add_child_data(other_data)

If you now re-run analysis, these added experiments will be copied but not modified, while the actual component experiments will have their data regenerated and analysis rerun (clearing all previous data and analysis results):

par_data2 = par_exp.run_analysis(par_data).block_for_results()

Same if you do replace results the extra child experiments that aren't from the composite experiment are just left unchanged, while the component experiments will have their data cleared and re-generated, and any existing analysis results updated by the re-run of analysis.

par_exp.run_analysis(par_data2, replace_results=True).block_for_results()

[yaelbh]

Thanks for the answer. My question also refers to internal implementation. Suppose that in some location in the code, there is a bug because some metadata field is not updated while it should have been updated. As a reviewer (and future maintainer) I have no way to catch it, because I'm not aware of this metadata field in the first place... This is why I'm asking for a description of the classes involved, their attributes and metadata keys, and how (if everything works correctly) they're supposed to change following operations.

I agree that it's more robust than before. But this effort (made for the sake of robustness) to identify when to copy instead of modifying, and the operations themselves of copying and modifying, introduce complexity to the code (without competition with how it was before 😄).

[nkanazawa1989]

Looks good to me.

[chriseclectic]

@yaelbh I agree with these issues regarding metadata and it not being very clear what is needed and where it is used in the internal code. Me and @nkanazawa1989 have been discussing some ideas for how to improve this, but that will be tackled in a separate follow-up PR since it is more complex and will effect all experiments. The hope is having his PR in place first will make that easier to start since there is only one ExperimentData class that needs updating, and composite experiments are updated to behave more like regular experiments with this more standard separation of job execution and data analysis.

[yaelbh]

Metadata keys are particularly difficult because they're kind of hiding, like kwargs. I find it confusing (not only in this PR but also more generally in qiskit-experiments and even in Qiskit) that properties are sometimes represented as class attributes, and sometimes as dictionary items in the metadata attributes. I don't always follow why a certain property is represented as an item in the metadata. It seems to me that attributes should be preferred if possible because they're more structured and explicit.

[nkanazawa1989]

I agree. But currently this dict is used everywhere in the codebase. If we want to change it, we need to review tons of (extra) files for minor change. Documentation for dict keys in this PR is not necessary because later we can replace it with more structured data.

[yaelbh]

Ok, so don't change the code, but if you can document the current situation, this will be very helpful. Another option, if you and @nkanazawa1989 plan to change the code very soon, and are confident that the current code is ok, and don't want to spend the effort of documenting something that will soon be removed, is that I dismiss my review. But please pick this option only if you're really going to make these changes soon.

[nkanazawa1989]

Yeah probably we can write an issue to track this @chriseclectic

Follow-up work