-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add Job Splitting Information Methods for Better User Experience #1281
Add Job Splitting Information Methods for Better User Experience #1281
Conversation
Hi @coruscating this PR is for issue #1247, which you assigned me, I could not push into PR #1248 and got confused about what to do so I created this PR please review it. |
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.
Thanks for updating. You still need to add a release note: https://github.com/Qiskit-Extensions/qiskit-experiments/blob/main/CONTRIBUTING.md#adding-a-new-release-note
dict: A dictionary containing information about job distribution. | ||
- "Total number of circuits in the experiment": Total number of | ||
circuits in the experiment. | ||
- "Maximum number of circuits": Maximum number of circuits in | ||
one Job | ||
- "Total number of jobs": Number of jobs needed for | ||
distribution. |
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.
This is still not building correctly, I suggest this:
dict: A dictionary containing information about job distribution. | |
- "Total number of circuits in the experiment": Total number of | |
circuits in the experiment. | |
- "Maximum number of circuits": Maximum number of circuits in | |
one Job | |
- "Total number of jobs": Number of jobs needed for | |
distribution. | |
dict: A dictionary containing information about job distribution. | |
- "Total number of circuits in the experiment": Total number of | |
circuits in the experiment. | |
- "Maximum number of circuits per job": Maximum number of circuits in | |
one job based on backend and experiment settings. | |
- "Total number of jobs": Number of jobs needed to run this experiment | |
on the currently set backend. |
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.
Almost there!
backend (Backend): The backend for which to get job distribution | ||
information. |
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.
backend (Backend): The backend for which to get job distribution | |
information. | |
backend: Optional, the backend for which to get job distribution | |
information. If not specified, the experiment must already have a set backend. |
if self.backend is None: | ||
raise QiskitError("A backend must be provided.") | ||
backend = self.backend | ||
self.backend = backend |
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.
One last thing: I don't think self.backend
should be set to backend
here. With the current logic, if job_info()
provides a new backend, then this backend will overwrite the current backend, which isn't behavior expected from a method that should just provide information. I think the PR can be merged after this line is removed.
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.
Hi @coruscating Sorry for the delay
I think this will fix it
if backend is None:
if self.backend is None:
raise QiskitError("A backend must be provided.")
backend = self.backend
# Get max circuits for job splitting
max_circuits_option = getattr(self.experiment_options, "max_circuits", None)
max_circuits_backend = backend.max_circuits
here are the results also I tried on test_framework.py
run successfully
{'Total number of circuits in the experiment': 200, 'Maximum number of circuits per job': 100, 'Total number of jobs': 2}
{'Total number of circuits in the experiment': 200, 'Maximum number of circuits per job': 50, 'Total number of jobs': 4}
{'Total number of circuits in the experiment': 200, 'Maximum number of circuits per job': 100, 'Total number of jobs': 2}
{'Total number of circuits in the experiment': 200, 'Maximum number of circuits per job': 110, 'Total number of jobs': 2}
[<qiskit_experiments.test.utils.FakeJob object at 0x0000025549DA24D0>, <qiskit_experiments.test.utils.FakeJob object at 0x0000025549DA3250>]
let me know if need further changes else I push the changes
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.
That looks good, thanks!
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.
Sorry, there's one more issue. backend.max_circuits
only works for V2 backends, which is why we have the BackendData
adaptor class that implements the max_circuits
property: https://github.com/Qiskit-Extensions/qiskit-experiments/blob/cfb47e24fae4173ea106de4b84e686a7933f8589/qiskit_experiments/framework/backend_data.py#L169-L176
So instead of max_circuits_backend = backend.max_circuits
you should do max_circuits_backend = BackendData(backend).max_circuits
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.
fixed
e4def66
to
cff4ad7
Compare
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.
Looks good to me, thanks for contributing!
…kit-community#1281) This PR aims to solve issue qiskit-community#1247 als the changes are similar to PR qiskit-community#1248 ### Summary: This pull request aims to enhance the user experience by providing clearer insights into the structure of experiments and their distribution across backends. It introduces two new public methods, `_max_circuits()` and `job_info(backend)`, while also refactoring the existing `_run_jobs()` method for better code organization and reuse of logic. ### Proposed Changes: Add` _max_circuits()` Method: This method will calculate the maximum number of circuits that can be included in a single job. This method will encapsulate the logic previously present in the ` _run_jobs()` method to determine the maximum circuits per job. Add `job_info(backend)` Method: This method will utilize the `_max_circuits()` method to provide users with information about the experiment's configuration for a specific backend. If the backend argument is provided, the method will return the number of circuits and the number of jobs the experiment will be split into on that backend. If the backend argument is not provided, the method will rely on the backend already specified in the experiment. Refactor `_run_jobs()` Method: The existing `_run_jobs()` method will be refactored to make use of the `_max_circuits()` method. This change will enhance code readability and maintainability by segregating the logic related to job distribution from the actual job execution logic. ### Testing Added `test_max_circuits` for testing of `_max_circuits()` and `test_job_info` for testing of `job_info(backend)` these tests ensure that methods functioning correctly --------- Co-authored-by: Helena Zhang <[email protected]>
This PR aims to solve issue #1247 als the changes are similar to PR #1248
Summary:
This pull request aims to enhance the user experience by providing clearer insights into the structure of experiments and their distribution across backends. It introduces two new public methods,
_max_circuits()
andjob_info(backend)
, while also refactoring the existing_run_jobs()
method for better code organization and reuse of logic.Proposed Changes:
Add
_max_circuits()
Method:This method will calculate the maximum number of circuits that can be included in a single job. This method will encapsulate the logic previously present in the
_run_jobs()
method to determine the maximum circuits per job.Add
job_info(backend)
Method:This method will utilize the
_max_circuits()
method to provide users with information about the experiment's configuration for a specific backend. If the backend argument is provided, the method will return the number of circuits and the number of jobs the experiment will be split into on that backend. If the backend argument is not provided, the method will rely on the backend already specified in the experiment.Refactor
_run_jobs()
Method:The existing
_run_jobs()
method will be refactored to make use of the_max_circuits()
method. This change will enhance code readability and maintainability by segregating the logic related to job distribution from the actual job execution logic.Testing
Added
test_max_circuits
for testing of_max_circuits()
andtest_job_info
for testing ofjob_info(backend)
these tests ensure that methods functioning correctly