Setting type for composite experiment #1296

nayan2167 · 2023-10-28T18:49:09Z

Summary

This PR solves issue #1289

Added experiment_type property and setter in BatchExperiment and ParallelExperiment also added experiment_type setter in BaseExperiment, this changes will provide user flexibility and ease of use within their applications.

PR checklist (delete when all criteria are met)

I have read the contributing guide CONTRIBUTING.md.
I have added the tests to cover my changes.
I have updated the documentation accordingly.
I have added a release note file using reno if this change needs to be documented in the release notes.

yaelbh

Thank you for working on this 😄 .

yaelbh · 2023-10-31T18:10:03Z

qiskit_experiments/framework/composite/batch_experiment.py

@@ -65,6 +66,9 @@ def __init__(
                      provided this will be initialized automatically from the
                      supplied experiments.
        """
+        # Experiment identification metadata
+        self._type = experiment_type if experiment_type else type(self).__name__


This will happen in the parent's constructor, what you have to do is to pass experiment_type to the parent's constructor.

By the way, in BaseExperiment you can modify this line to self.experiment_type = experiment_type.

lol I should have read docs first, fixed it, removed proparty and setter from both BaseExperiment and ParallelExperiment added init arg experiment_type in both.

Remove this line.
Then, in BaseExperiment, replace this line with self.experiment_type = experiment_type.

yaelbh · 2023-10-31T18:11:04Z

qiskit_experiments/framework/composite/batch_experiment.py

@@ -89,6 +93,19 @@ def __init__(
            experiments, qubits, backend=backend, analysis=analysis, flatten_results=flatten_results
        )

+    @property


These property and setter already exist in BaseExperiment, so there is no reason to copy them here.

Note that all the comments for the BatchExperiment apply also to the ParallelExperiment.

What do you mean by "fixed"? Did you commit the fix? Because the property and setter are still here.

yaelbh · 2023-10-31T18:20:07Z

releasenotes/notes/setter-methods-for-experiment-099074e59faffb49.yaml

+---
+features:
+  - |
+    Added ``experiment_type`` as optional ``__init__`` kwarg in BatchExperiment


There are two separate things in this PR: the setter and the args in init. You can separate them in the release notes.

yaelbh · 2023-10-31T18:20:33Z

releasenotes/notes/setter-methods-for-experiment-099074e59faffb49.yaml

+    and ParallelExperiment the 'experiment_type' can now be easily set and 
+    retrieved from the experiment object post-construction using the 'experiment_type'
+    property and setter.
+    :math":`BatchExperiment.experiment_type` as property and setter.


Change after fixing batch_experiment.py.

yaelbh · 2023-10-31T18:20:50Z

releasenotes/notes/setter-methods-for-experiment-099074e59faffb49.yaml

+  - |
+    :math:`BaseExperiment.experiment_type` as setter.
+  - |
+    Refer issue 


@coruscating Do we usually refer to issues in the release notes?

yaelbh · 2023-10-31T18:22:34Z

test/framework/test_composite.py

@@ -151,6 +152,29 @@ def test_roundtrip_serializable(self):

        self.assertRoundTripSerializable(exp)

+    @data(None, "Test")
+    def test_experiment_type(self, exp_type):
+        """Test the experiment_type setter for the parallel experiment and


Once you remove the setters from these classes, you can also remove the test here. However do test the the init arg (such a test is currently missing).

test/framework/test_framework.py

yaelbh · 2023-10-31T18:26:43Z

test/framework/test_framework.py

+            def circuits(self):
+                pass
+
+        exp1 = MyExp(physical_qubits=[0], experiment_type=exp_type1)


Instead of passing exp_type to the constructor, pass some arbitrary string (like blaaa) and verify that the setter overrides it.

yaelbh · 2023-10-31T18:27:48Z

test/framework/test_framework.py

+
+        exp1 = MyExp(physical_qubits=[0], experiment_type=exp_type1)
+        if exp_type1 is None:
+            exp_type1 = type(exp1).__name__


I don't understand what you're trying to test here. Note that you're not testing the case where the setters gets None.

like this?

def test_experiment_type(self): """Test the experiment_type setter for the experiment.""" class MyExp(BaseExperiment): """Some arbitrary experiment""" def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def circuits(self): pass exp1 = MyExp(physical_qubits=[0], experiment_type="blaaa") self.assertEqual(exp1.experiment_type, "blaaa") exp2 = MyExp(physical_qubits=[0]) exp2.experiment_type = "suieee" self.assertEqual(exp2.experiment_type, "suieee")

Yes, and between the third and fourth line add:

self.assertEqual(exp2.experiment_type, "MyExp")

yaelbh · 2023-11-01T09:05:52Z

qiskit_experiments/framework/composite/batch_experiment.py

@@ -65,6 +66,9 @@ def __init__(
                      provided this will be initialized automatically from the
                      supplied experiments.
        """
+        # Experiment identification metadata
+        self._type = experiment_type if experiment_type else type(self).__name__


Remove this line.
Then, in BaseExperiment, replace this line with self.experiment_type = experiment_type.

yaelbh · 2023-11-01T09:06:48Z

qiskit_experiments/framework/composite/batch_experiment.py

@@ -89,6 +93,19 @@ def __init__(
            experiments, qubits, backend=backend, analysis=analysis, flatten_results=flatten_results
        )

+    @property


What do you mean by "fixed"? Did you commit the fix? Because the property and setter are still here.

nayan2167 · 2023-11-01T19:07:57Z

I have got this same output for both cases

ParallelExperiment
--------------------
suiee
--------------------
BatchExperiment
--------------------
blaa
--------------------

qiskit_experiments/framework/composite/parallel_experiment.py

yaelbh · 2023-11-02T12:38:37Z

releasenotes/notes/setter-methods-for-experiment-099074e59faffb49.yaml

+---
+features:
+  - |
+    Added ``experiment_type`` as optional ``__init__`` kwarg in BatchExperiment


Mark BatchExperiment as a class

yaelbh · 2023-11-02T12:38:57Z

releasenotes/notes/setter-methods-for-experiment-099074e59faffb49.yaml

+    Added ``experiment_type`` as optional ``__init__`` kwarg in BatchExperiment
+    and ParallelExperiment.
+  - |
+    :math:`BaseExperiment.experiment_type` as setter,'experiment_type' can now


Suggested change

:math:`BaseExperiment.experiment_type` as setter,'experiment_type' can now

'experiment_type' can now

…b.com/nayan2167/qiskit-experiments into Setting-type-for-composite-experiment

yaelbh · 2023-11-05T11:06:42Z

The code looks fine. I don't understand why the docs don't build. I'll try to rerun.

coruscating · 2023-11-06T20:31:06Z

CI is fixed now.

### Summary This PR solves issue qiskit-community#1289 Added `experiment_type` property and setter in `BatchExperiment` and `ParallelExperiment` also added `experiment_type` setter in `BaseExperiment`, this changes will provide user flexibility and ease of use within their applications. ### PR checklist (delete when all criteria are met) - [X] I have read the contributing guide `CONTRIBUTING.md`. - [X] I have added the tests to cover my changes. - [X] I have updated the documentation accordingly. - [X] I have added a release note file using `reno` if this change needs to be documented in the release notes. --------- Co-authored-by: Helena Zhang <Helena.Zhang@ibm.com>

yaelbh self-requested a review October 31, 2023 18:05

yaelbh requested changes Oct 31, 2023

View reviewed changes

yaelbh requested changes Nov 1, 2023

View reviewed changes

nayan2167 added 3 commits November 2, 2023 00:25

Add experiment_type setter method

ce7a0d9

Add experiment_type args

b432129

Add relese notes

ec9686f

nayan2167 force-pushed the Setting-type-for-composite-experiment branch from ce63ed3 to ec9686f Compare November 1, 2023 18:57

Merge branch 'main' into Setting-type-for-composite-experiment

0e89952

yaelbh reviewed Nov 2, 2023

View reviewed changes

qiskit_experiments/framework/composite/parallel_experiment.py Show resolved Hide resolved

yaelbh reviewed Nov 2, 2023

View reviewed changes

nayan2167 added 4 commits November 2, 2023 23:26

Update cunstructor logic

ab7e5b8

Update relesenotes

697a99d

Merge branch 'Setting-type-for-composite-experiment' of https://githu…

657eb1c

…b.com/nayan2167/qiskit-experiments into Setting-type-for-composite-experiment

Add test for experiment_type setter

af2eaee

Merge branch 'main' into Setting-type-for-composite-experiment

dfc4728

yaelbh approved these changes Nov 7, 2023

View reviewed changes

yaelbh added this pull request to the merge queue Nov 7, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 7, 2023

coruscating added this pull request to the merge queue Nov 9, 2023

Merged via the queue into qiskit-community:main with commit d4b9d14 Nov 9, 2023
10 checks passed

nayan2167 deleted the Setting-type-for-composite-experiment branch November 21, 2023 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting type for composite experiment #1296

Setting type for composite experiment #1296

nayan2167 commented Oct 28, 2023

yaelbh left a comment

yaelbh Oct 31, 2023

nayan2167 Oct 31, 2023

yaelbh Nov 1, 2023

yaelbh Oct 31, 2023

nayan2167 Oct 31, 2023

yaelbh Nov 1, 2023

yaelbh Oct 31, 2023

yaelbh Oct 31, 2023

yaelbh Oct 31, 2023

yaelbh Oct 31, 2023

yaelbh Oct 31, 2023

yaelbh Oct 31, 2023

nayan2167 Oct 31, 2023

yaelbh Nov 2, 2023

yaelbh Nov 1, 2023

yaelbh Nov 1, 2023

nayan2167 commented Nov 1, 2023

yaelbh Nov 2, 2023

yaelbh Nov 2, 2023

yaelbh commented Nov 5, 2023

coruscating commented Nov 6, 2023

	:math:`BaseExperiment.experiment_type` as setter,'experiment_type' can now
	'experiment_type' can now

Setting type for composite experiment #1296

Setting type for composite experiment #1296

Conversation

nayan2167 commented Oct 28, 2023

Summary

PR checklist (delete when all criteria are met)

yaelbh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayan2167 commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaelbh commented Nov 5, 2023

coruscating commented Nov 6, 2023