FigureData wrapper for figures #814

gadial · 2022-06-02T12:55:41Z

Summary

This PR adds a FigureData wrapper around figures added to ExperimentData to enable saving additional metadata (e.g. qubits) with the figure. This data is currently not saved in the result database.

Details and comments

…is result

yaelbh

Still missing:

Save the figure and not the data class (I could run in my environment after I made a dirty fix).
Sync the name data member with the figure_names array. For example, when the user modifies the figure's name, by modifying one of them. Alternatively block such modifications.
Tests.

gadial · 2022-06-07T11:32:00Z

Still missing:

Save the figure and not the data class (I could run in my environment after I made a dirty fix).

Sync the name data member with the figure_names array. For example, when the user modifies the figure's name, by modifying one of them. Alternatively block such modifications.

Tests.

All fixed. For name I chose to make it read-only for now; if we want change to name to change the containing ExperimentData figure list, we can, but it's better to avoid doing it if there is no need for this feature.

qiskit_experiments/database_service/db_experiment_data.py

yaelbh · 2022-06-08T07:35:10Z

qiskit_experiments/database_service/db_experiment_data.py

+        self._name = name
+        self.metadata = metadata if metadata is not None else {}
+
+    # name is read only


Are there potential issues with the user modifying figure_names after a figure was created?

I was wondering this too. In theory couldn't you rename a figure by using this?

I expect it would have some headaches in how you propagate it through to the DB since you would likely need to track the old figure name to delete when saving under the new figure name.

I agree; so for now, the user cannot change name.

Co-authored-by: Yael Ben-Haim <[email protected]>

chriseclectic

A few suggestions, but I think this is headed in the right direction

qiskit_experiments/database_service/db_experiment_data.py

chriseclectic · 2022-06-09T14:00:43Z

qiskit_experiments/database_service/db_experiment_data.py

+        self._name = name
+        self.metadata = metadata if metadata is not None else {}
+
+    # name is read only


I was wondering this too. In theory couldn't you rename a figure by using this?

I expect it would have some headaches in how you propagate it through to the DB since you would likely need to track the old figure name to delete when saving under the new figure name.

chriseclectic · 2022-06-09T14:02:10Z

qiskit_experiments/database_service/db_experiment_data.py

@@ -130,6 +129,36 @@ def __json_encode__(self):
        return self.__getstate__()


+class FigureData:


This could probably just be called Figure.

I would also add a __repr__ or _repr_html_ method that prints the name and displays the actual figure (not exactly sure how to do this). This should work whether the figure is a MPL figure, or a loaded SVG from database (later can is IPython.SVG I think).

Figure is already taken by pyplot

Using Figure will cause confusion with pyplot's figure; also, this requires changing the way figures are referenced all over the code.

chriseclectic · 2022-06-09T14:08:47Z

qiskit_experiments/database_service/db_experiment_data.py

+    def copy(self, new_name: Optional[str] = None):
+        """Creates a deep copy of the figure data"""
+        name = new_name if new_name is not None else self.name
+        return FigureData(figure=self.figure, name=name, metadata=copy.deepcopy(self.metadata))


This isn't a deep copy if the figure isn't also deep copied. If you want a deepcopy why not just do copy.deepcopy(self) ?

This method is needed since we don't allow yet changing the name, but when copying might want to change it. I'll deepcopy the figure as well.

Just tried it and deep copying the figure causes matplotlib errors.

chriseclectic · 2022-06-09T14:12:23Z

qiskit_experiments/database_service/db_experiment_data.py

+        """Initialize a figure data from the json representation"""
+        return cls(**args)
+
+


I think it could be nice to add a save and load method to this class for saving the the figure to a file or the DB.

While this wouldn't be supported yet a save method for DB would be useful if you change filename or add metadata and to save to update DB.

I don't understand what those functions should do now. Their existence might trick the user to think the data is saved to the DB.

qiskit_experiments/database_service/db_experiment_data.py

chriseclectic · 2022-06-09T14:15:19Z

qiskit_experiments/database_service/db_experiment_data.py

            self._figures[figure_key] = figure_data

        if figure_data is None:
            raise DbExperimentEntryNotFound(f"Figure {figure_key} not found.")

        if file_name:
            with open(file_name, "wb") as output:
-                num_bytes = output.write(figure_data)
+                num_bytes = output.write(figure_data.figure)


This writing to binary could be moved to the save method of Figure class

There should also be some LOG.warning msgs that if you try and save a figure with metadata that the metadata is not currently saved to DB

Added a warning, not sure about the save method; in this context we are saving the figure obtained from the DB into a file on the local computer.

qiskit_experiments/database_service/db_experiment_data.py

chriseclectic · 2022-06-09T14:22:45Z

releasenotes/notes/figure_data-ecf5a82c95844b6a.yaml

+---
+features:
+  - |
+    Figures added to `ExperimentData` are now wrapped with a `FigureData` class


This might be better as an upgrade note. Something like:

Adds a :class:`.FigureData` class for adding metadata to a analysis result figures. Figures added to `ExperimentData` are now stored using this class. The raw image object (SVG or matplotlib.Figure) can be accessed using the :attr:`.FigureData.figure` attribute. Note that currently metadata is only stored locally and will be discarded when saved to the cloud experiment service database.

chriseclectic

Looks good. Couple minor changes inc adding the new class to the qiskit_experiments.framework API docs.

qiskit_experiments/database_service/db_experiment_data.py

chriseclectic · 2022-06-20T18:57:45Z

qiskit_experiments/database_service/db_experiment_data.py

+        """Initialize a figure data from the json representation"""
+        return cls(**args)
+
+    def _repr_png_(self):


What is the reason for having both png and svg reprs? Does svg repr not work well for MPL figures?

Yes, with svg sometimes there were missing things from the diagram. I played with it and it seems to me png was the best method I could find to consistently give good results.

chriseclectic · 2022-06-20T18:58:29Z

qiskit_experiments/database_service/db_experiment_data.py

+
+    def _repr_png_(self):
+        if isinstance(self.figure, MatplotlibFigure):
+            b = io.BytesIO()


I wonder if this should be cached so the conversion doesn't happen everytime you view it.

Not sure we need this - it is very quick, and we don't display many images at once, certainty not several times for the same image.

qiskit_experiments/database_service/db_experiment_data.py

chriseclectic · 2022-06-20T19:06:55Z

qiskit_experiments/database_service/db_experiment_data.py

+                # currently only the figure and its name are stored in the database
+                if isinstance(figure, FigureData):
+                    figure = figure.figure
+                    LOG.warning("Figure metadata is currently not saved to the database")


Suggested change

LOG.warning("Figure metadata is currently not saved to the database")

if figure.metadata:

LOG.warning(f"Figure {figure.name} metadata cannot be saved to the experiment service.")

This only needs warning if the metadata is not empty, otherwise there will be warnings raised everywhere.

Technically you are correct, but in practice the metadata will never be empty, since it has a qubits field with the physical_qubits from the experiment itself (this was needed for qiskit-monitoring, the initial motivation for this PR). Also, it probably should always be initialized and not None even without it.

So right now we will get warnings everywhere, even with your suggested fix. Not sure what's the best way to go about this - I'm not sure we need this warning.

releasenotes/notes/figure_data-ecf5a82c95844b6a.yaml

Co-authored-by: Christopher J. Wood <[email protected]>

chriseclectic

It looks like the wrong logger msg was changed from warning to debug

qiskit_experiments/database_service/db_experiment_data.py

* Added FigureData wrapper for figures * FigureData is now kept when adding the figure into a composite analysis result * Update qiskit_experiments/database_service/db_experiment_data.py Co-authored-by: Yael Ben-Haim <[email protected]> Co-authored-by: Christopher J. Wood <[email protected]>

gadial added 2 commits June 2, 2022 15:54

Added FigureData wrapper for figures

2aabb72

Linting

1fd919a

gadial changed the title ~~FigureData wrapper for figures~~ [WIP] FigureData wrapper for figures Jun 2, 2022

gadial added 2 commits June 2, 2022 18:57

Bugfix

ea4fb11

Release note

120a5d8

gadial requested a review from yaelbh June 6, 2022 06:51

FigureData is now kept when adding the figure into a composite analys…

3a0aaaa

…is result

yaelbh reviewed Jun 7, 2022

View reviewed changes

gadial added 4 commits June 7, 2022 13:27

Fix in saving and making name read only

484b3d7

Figure metadata tests

ab2d62e

copy and serialization for figure data

8fc9abd

Linting

b9d48ec

Removing unused variable

755258b

gadial changed the title ~~[WIP] FigureData wrapper for figures~~ FigureData wrapper for figures Jun 7, 2022

yaelbh approved these changes Jun 8, 2022

View reviewed changes

Update qiskit_experiments/database_service/db_experiment_data.py

975b2cd

Co-authored-by: Yael Ben-Haim <[email protected]>

chriseclectic requested changes Jun 9, 2022

View reviewed changes

gadial added 8 commits June 13, 2022 17:40

Figure metadata setter/getter

de2816d

Deep copy the figure

4fcb249

Added repr method for FigureData

459cece

Small fixes and release note change

8c6918f

Linting

0ea57a7

Merge branch 'main' into figure_data

b2f5d33

Removing image_only keyword

6a3c728

Test fixes for the new FigureData interface

3e66159

chriseclectic requested changes Jun 20, 2022

View reviewed changes

gadial and others added 3 commits June 21, 2022 10:07

Update qiskit_experiments/database_service/db_experiment_data.py

53a0a33

Co-authored-by: Christopher J. Wood <[email protected]>

Update qiskit_experiments/database_service/db_experiment_data.py

6f83017

Co-authored-by: Christopher J. Wood <[email protected]>

Update releasenotes/notes/figure_data-ecf5a82c95844b6a.yaml

366dcf6

Co-authored-by: Christopher J. Wood <[email protected]>

gadial and others added 3 commits June 21, 2022 10:30

Update releasenotes/notes/figure_data-ecf5a82c95844b6a.yaml

d6ea3ac

Co-authored-by: Christopher J. Wood <[email protected]>

Added FigureData to framework's init and docs

f5efc89

Moved warning over figure data not saved in db to debug level

5bc917d

chriseclectic requested changes Jun 22, 2022

View reviewed changes

qiskit_experiments/database_service/db_experiment_data.py Outdated Show resolved Hide resolved

qiskit_experiments/database_service/db_experiment_data.py Outdated Show resolved Hide resolved

Fix Log warning -> debug

9728d05

chriseclectic approved these changes Jun 24, 2022

View reviewed changes

chriseclectic merged commit 1b1ea01 into qiskit-community:main Jun 24, 2022

gadial deleted the figure_data branch June 24, 2022 15:57

mriedem mentioned this pull request Jun 27, 2022

Figure integration tests are failing with "AssertionError: <qiskit_experiments.database_service.db_e[46 chars]7130> != b'hello world'" since Friday June 24 #831

Closed

coruscating modified the milestone: Release 0.4 Jul 12, 2022

coruscating mentioned this pull request Jul 21, 2022

Figures not shown in Quantum Volume tutorial #855

Closed

wshanks mentioned this pull request Nov 27, 2023

Make figures loaded from experiment db viewable in notebook #1321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FigureData wrapper for figures #814

FigureData wrapper for figures #814

gadial commented Jun 2, 2022

yaelbh left a comment

gadial commented Jun 7, 2022

yaelbh Jun 8, 2022

chriseclectic Jun 9, 2022

gadial Jun 13, 2022

chriseclectic left a comment

chriseclectic Jun 9, 2022

chriseclectic Jun 9, 2022

chriseclectic Jun 9, 2022

yaelbh Jun 9, 2022

gadial Jun 13, 2022

chriseclectic Jun 9, 2022

gadial Jun 13, 2022

gadial Jun 13, 2022

chriseclectic Jun 9, 2022

gadial Jun 13, 2022

chriseclectic Jun 9, 2022

chriseclectic Jun 9, 2022

gadial Jun 20, 2022

chriseclectic Jun 9, 2022

chriseclectic left a comment

chriseclectic Jun 20, 2022

gadial Jun 21, 2022

chriseclectic Jun 20, 2022

gadial Jun 21, 2022

chriseclectic Jun 20, 2022

gadial Jun 21, 2022

chriseclectic left a comment

		@@ -130,6 +129,36 @@ def __json_encode__(self):
		return self.__getstate__()


		class FigureData:

		"""Initialize a figure data from the json representation"""
		return cls(**args)

	LOG.warning("Figure metadata is currently not saved to the database")
	if figure.metadata:
	LOG.warning(f"Figure {figure.name} metadata cannot be saved to the experiment service.")

FigureData wrapper for figures #814

FigureData wrapper for figures #814

Conversation

gadial commented Jun 2, 2022

Summary

Details and comments

yaelbh left a comment

Choose a reason for hiding this comment

gadial commented Jun 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseclectic 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

chriseclectic 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

chriseclectic left a comment

Choose a reason for hiding this comment