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

Address timing dependence in ExperimentData.add_jobs test #1267

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

wshanks
Copy link
Collaborator

@wshanks wshanks commented Sep 7, 2023

Previously, the test test_add_data_job called add_jobs with two jobs and then tested that the results from ExperimentData.data() matched the results of the first job concatenated with the second. ExperimentData uses threads to handle tracking jobs. Even though these jobs are dummy jobs that take no time, ExperimentData would occasionally add the results from the second job before the first and then fail the results comparison. With this change, a label is attached to each result and then the results are sorted based on the label before testing against the expected results.

Closes #1044

Previously, the test `test_add_data_job` called `add_jobs` with two jobs
and then tested that the results from `ExperimentData.data()` matched
the results of the first job concatenated with the second.
`ExperimentData` uses threads to handle tracking jobs. Even though these
jobs are dummy jobs that take no time, `ExperimentData`  would
occasionally add the results from the second job before the first and
then fail the results comparison. With this change, a label is attached
to each result and then the results are sorted based on the label
before testing against the expected results.

Closes qiskit-community#1044
@wshanks wshanks added the Changelog: None Do not include in changelog label Sep 7, 2023
@wshanks
Copy link
Collaborator Author

wshanks commented Sep 7, 2023

I thought I would fix this since it had the milestone on it. From what I could see, there was only one test in test_db_experiment_data.py that was affected by the issue (called add_jobs with multiple jobs). I think this is small enough that we don't need a release note.

Copy link
Collaborator

@coruscating coruscating left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for fixing it. Just a minor question.

@@ -1073,7 +1081,7 @@ def _job2_result():
exp_data.data(0)["counts"], [copied.data(0)["counts"], copied.data(1)["counts"]]
)

def _get_job_result(self, circ_count, has_metadata=False):
def _get_job_result(self, circ_count, label_from=0, no_metadata=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change has_metadata to no_metadata? Also, this changes the default behavior of _get_job_result() to always return metadata? I guess this didn't affect other tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

has_metadata was only used by one test to test the behavior when a result has metadata versus when it does not have metadata. label_from adds metadata. I thought it was cleaner for label_from to work by default, unless you requested no_metdata than to require label_from and has_metadata for that case. I don't feel strongly though. The two tests I modified are the only ones in the module that care about labels or other metadata.

@coruscating coruscating added this pull request to the merge queue Sep 7, 2023
Merged via the queue into qiskit-community:main with commit 4f08d6f Sep 7, 2023
22 checks passed
@wshanks wshanks deleted the test-result-ordering branch September 8, 2023 12:12
nkanazawa1989 pushed a commit to nkanazawa1989/qiskit-experiments that referenced this pull request Jan 10, 2024
…munity#1267)

Previously, the test `test_add_data_job` called `add_jobs` with two jobs
and then tested that the results from `ExperimentData.data()` matched
the results of the first job concatenated with the second.
`ExperimentData` uses threads to handle tracking jobs. Even though these
jobs are dummy jobs that take no time, `ExperimentData` would
occasionally add the results from the second job before the first and
then fail the results comparison. With this change, a label is attached
to each result and then the results are sorted based on the label before
testing against the expected results.

Closes qiskit-community#1044
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExperimentData tests assume that .data() is ordered
2 participants