-
Notifications
You must be signed in to change notification settings - Fork 27
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
[INS-126] Project names in Qiskit #92
Conversation
QFer
commented
Feb 5, 2021
- When using Qiskit, now also Quantum Inspire project names can be set
- When no project name is set, a default is taken
- When an existing project name is chosen, the experiments are added to the existing project
- qi_job.result() now has a parameter to only fetch the results for the latest run (instead ALL results for the project)
src/tests/quantuminspire/qiskit/test_quantum_inspire_provider.py
Outdated
Show resolved
Hide resolved
@@ -541,7 +596,7 @@ def run_histogram_test(self, single_experiment, mock_result1, mock_result2, expe | |||
self.mock_api.get_jobs_from_project.return_value = [jobs] | |||
job = QIJob('backend', '42', self.mock_api) | |||
|
|||
result = self.simulator.get_experiment_results(job) | |||
result = self.simulator.get_experiment_results(job, False) |
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.
The meaning of the second argument is not clear. Consider using the explicit parameter name: only_latest_run=False
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.
Done
src/quantuminspire/qiskit/qi_job.py
Outdated
@@ -66,12 +77,14 @@ def submit(self) -> None: | |||
raise JobError('Job has already been submitted!') | |||
self._job_id = self._backend.run(self._qobj) | |||
|
|||
def result(self, timeout: Optional[float] = None, wait: float = 0.5) -> QIResult: | |||
def result(self, timeout: Optional[float] = None, wait: float = 0.5, only_latest_run: bool = True) -> QIResult: |
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.
Similar to the comment on get_experiment_results
, you might want to consider removing the additional argument, and in stead create a new function all_results_from_project
, or something similarly named.
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.
I can do that but this QIJob class is subclassed from a Qiskit class BaseJob where the interface is specified for getting results (job.result()) in Qiskit. I want to conform to this interface instead of extending it. Of course I can extend the interface and add a new all_results_from_project but the users won't easily find it and use it because they are known to the Qiskit interface for getting back the results. It's described this way in the Qiskit documentation etc. Only the ones who dive into it a bit and really understand how Quantum Inspire works with projects and jobs, may change (one of) the parameter of result().
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.
Hmm. I'm not sure that this argument holds. You are already extending the interface by introducing a new boolean flag as function parameter. A flag parameter to make the function behave differently is actually a well-known code smell. The recommended refactoring for that is to create a new function for the different behavior.
And in both cases (flag parameter or new function) the user must have done some research before they can use 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.
I'll add another method for getting the results for all runs: result_all_jobs