pulse backend string model parser by DanPuzzuoli · Pull Request #71 · qiskit-community/qiskit-dynamics

DanPuzzuoli · 2021-12-10T20:26:52Z

Summary

Closes #53.

Bring string model parser from Aer into dynamics. Currently a work in progress as I attempt to reduce the original number of files and lines of code involved.

Details and comments

…nts with operators

…nel is not specified in required format. Added tests for this and other others

…some string models

… as its no longer needed

… -> operator conversion

…n of what the function can be used to generate. Added test cases for this function

…checking well-formed strings

…x] string format

brosand

Looking good, mostly just left a couple small suggestions for testing and parsing, and a couple small typos.

DanPuzzuoli · 2022-02-08T21:40:38Z

Not sure why I can't reply to the one comment about adding the output to one of the examples in the docs.

I'm kind of on the fence about this - two of the outputs of the function consist of arrays and so showing them isn't necessarily too informative. Given that this function is primarily for internal use anyway I'm leaning towards not adding this.

brosand · 2022-02-08T22:16:39Z

Not sure why I can't reply to the one comment about adding the output to one of the examples in the docs.

I'm kind of on the fence about this - two of the outputs of the function consist of arrays and so showing them isn't necessarily too informative. Given that this function is primarily for internal use anyway I'm leaning towards not adding this.

Not sure why I can't reply to this one either haha, but this makes sense,

brosand · 2022-02-08T22:17:52Z

        self.assertTrue("does not conform" in str(qe.exception))

+    def test_single_vertical_bars(self):
+        """Test that too many vertical bars raises error."""


should be "test that too few vertical bars"

brosand

LGTM, thanks for doing those small changes

chriseclectic

Since majority of this is intended as internal code hidden from users i would recommend adding some comments to that effect in the module docstrings, and renaming most of the classes and functions to start with an underscore to further indicate they are internal methods that should not be used directly

chriseclectic · 2022-02-11T15:31:06Z

 """

 from .pulse_to_signals import InstructionToSignals
+from .string_model_parser.string_model_parser import parse_hamiltonian_dict


Is this function supposed to be directly called by a user? I thought it was more an internal function that is planned to be used by some yet-to-be-made interface for building models from ibm backend configs?

You're right, I'll remove this.

chriseclectic · 2022-02-11T15:32:08Z

+from .operator_from_string import operator_from_string, apply_func
+
+
+def legacy_parser(operator_str, subsystem_dims, subsystem_list):


Why is this called legacy_parser? To me legacy implies there is a newer version that that this is planned to be removed at some point

This function should have type hints and doc string to say what is expected for arg types, and the return

I'm using "legacy" here to imply that it's a non-trivial piece of code that is being preserved but not "maintained" per se. I like the usage of "legacy" to imply this - I think the way you're interpreting it is basically synonymous with "expected to be deprecated".

That all being said I'll change this if it's a sticking point for you.

Updated the function to have type hints.

chriseclectic · 2022-02-11T15:33:12Z

+
+Token = namedtuple("Token", ("type", "name"))
+
+str_elements = OrderedDict(


If this is only used by HamiltonianParser it should be class variable of that class, no need for global scope. Same for Token

Moved this into the class.

chriseclectic · 2022-02-11T15:34:11Z

+        """
+        self.h_str = h_str
+        self.subsystem_dims = {int(label): int(dim) for label, dim in subsystem_dims.items()}
+        self.__td_hams = []


Why double underscore?

No reason, it was in the original code. I've changed it to just td_hams.

chriseclectic · 2022-02-11T15:35:56Z

+)
+
+
+class HamiltonianParser:


Is there any need for this to be a class instead of a giant function? If the only way you use it is via a legacy_parser call you could just merge the init + parse + compiled into a single function since there is no reason to save state in an objects variables, all those can just be local variables in the functions scope

Might also want to call this _HamiltonianParser just to add further indication this shouldn't be used and is internal only

No reason - this is part of the "legacy" aspect of it and why I wrote the legacy_parser function. Turning it directly into a function was non-trivial given the handling of internal state. The legacy_parser function was added to wrap this in the preferred standard function interface.

chriseclectic · 2022-02-11T15:56:41Z

+
+
+# pylint: disable=invalid-name
+__funcdict = {"dag": dag}


What is the point of this?

chriseclectic · 2022-02-11T16:02:33Z

+__funcdict = {"dag": dag}
+
+
+def apply_func(name: str, op: np.ndarray) -> np.ndarray:


This function seems sus. You could just replace it in the parser code (which is the only spot it looks to be used) with something like

elif token.type in ["Func", "Ext"]: if token.name == "dag": stack.append(np.conjugate(np.transpose(stack.pop(-1))) else: stack.append(stack.pop(-1))

(possibly something even simpler if you know whats going on in that code block)

yeah you're right - I think maybe it was like this in the original pulse simulator to enable further expansion of functionality later, but for now it's pointless. I've implemented your suggestion and removed apply_func and related things, though changed the else statement to raise an error.

chriseclectic · 2022-02-11T16:04:51Z

+    subsystem_dims = {int(qubit): qub_dict[int(qubit)] for qubit in subsystem_list}
+
+    # Parse the Hamiltonian
+    system = legacy_parser(


No need for this function, it could just be system = HarmiltonianParser(args).parse(other_args)

Unless you think otherwise I'd rather keep the legacy_parser function even though this is all it does as its the preferred interface (a function call).

chriseclectic · 2022-02-11T16:07:30Z

+    return np.diag(np.sqrt(np.arange(1, dim, dtype=complex)), 1)
+
+
+def adag(dim: int) -> np.ndarray:


Same with all these functions, could call them _a, _X etc

chriseclectic · 2022-02-11T16:08:01Z

+    return static_hamiltonian, list(hamiltonian_operators), list(reduced_channels), subsystem_dims
+
+
+def hamiltonian_pre_parse_exceptions(hamiltonian_dict: dict):


Suggested change

def hamiltonian_pre_parse_exceptions(hamiltonian_dict: dict):

def _hamiltonian_pre_parse_exceptions(hamiltonian_dict: dict):

DanPuzzuoli · 2022-02-11T18:44:11Z

@chriseclectic I think I've addressed most comments with the latest changes. The remaining things to discuss are:

Usage of the word "legacy". Maybe we can change the file to regex_parser.py and this function to regex_parser - this will drop the problematic terminology and may be a more informative name (indicating this is where the true "parsing" happens).
Using underscores for functions/classes that are meant to be internal: I'd prefer not to do this as I feel like it is unnecessary and I don't really like the way it looks. I think the fact that these things aren't being imported into the standard packaging structure/API, along with the warnings in the files, should be sufficient to imply that they are internal?

…ynamics into pulse_string_parser

DanPuzzuoli · 2022-11-22T17:20:42Z

Closing this PR as it is extremely out of date - will reopen a new one.

DanPuzzuoli added 15 commits November 11, 2021 10:50

starting point - bringing parser files over from Aer

6b1afb0

adding hamiltonian_model as well as it does some of the parsing

3689090

added file and sketched signature for desired function

58679be

trying things out - initial success with multiplying string coefficie…

8ffc260

…nts with operators

added error raising if more than one channel is specified, or if chan…

16a7419

…nel is not specified in required format. Added tests for this and other others

added operator merging and channel sorting step, and added tests for …

10e7cf6

…some string models

adding some documentation and removing the old hamiltonian_model file…

396b60a

… as its no longer needed

saving

dca82ae

initial tests passing with stripped down version of individual string…

65cd233

… -> operator conversion

added doc string from operator_from_string giving explicit descriptio…

3c1f830

…n of what the function can be used to generate. Added test cases for this function

saving

0f9a4c5

Merge branch 'main' into pulse_string_parser

dc328cf

converting output aggregated channel list to lower case

fd4a577

expanded valid channel characters and added test cases

a2f4eca

added more complicated test case and discovered issue with logic for …

7182fd3

…checking well-formed strings

DanPuzzuoli added the Changelog: New Feature Include in the "Added" section of the changelog label Dec 11, 2021

DanPuzzuoli added 12 commits December 13, 2021 12:53

restructured validation checks to allow _SUM strings

b960d16

deleting unused old files

c1f31d6

fully deleting all old files

a59be2f

intermediate linting

c934da4

further updating validation, and adding documentation note for _SUM[x…

9da584b

…x] string format

about to try noise parsing

7e5ed5d

adding noise parsing function and deleting old noise parser object

d17f77b

small changes

d61e507

moving files around

3394501

attempting to simplify and linting

6d5cdc7

adding examples to documentation

1291532

adding parse_hamiltonian_dict to pulse module documentation

7ff2d17

DanPuzzuoli marked this pull request as ready for review January 4, 2022 21:41

DanPuzzuoli changed the title ~~[WIP] String model parser~~ pulse backend string model parser Jan 4, 2022

brosand self-requested a review February 1, 2022 19:53

brosand reviewed Feb 7, 2022

View reviewed changes

DanPuzzuoli added 4 commits February 8, 2022 15:46

Merge branch 'main' into pulse_string_parser

dc8ea0f

Adding acquire channels to string parser

327d05d

fixing typos and adding tests addressing PR comments

eb6d23f

formatting

cee451a

DanPuzzuoli requested a review from brosand February 8, 2022 21:42

brosand reviewed Feb 8, 2022

View reviewed changes

correcting test comment

c26ffcf

DanPuzzuoli requested a review from brosand February 9, 2022 13:54

brosand previously approved these changes Feb 9, 2022

View reviewed changes

chriseclectic suggested changes Feb 11, 2022

View reviewed changes

DanPuzzuoli added 3 commits February 11, 2022 11:55

Merge branch 'main' into pulse_string_parser

6b647b2

updating formatting after merging main

ad74ed2

addressing most review comments

7e1dea3

DanPuzzuoli dismissed brosand’s stale review via 7e1dea3 February 11, 2022 18:40

DanPuzzuoli requested a review from chriseclectic February 11, 2022 18:45

DanPuzzuoli added 6 commits February 11, 2022 14:29

Merge branch 'main' into pulse_string_parser

524c7ce

updating operator_from_String to utilize quantum_info

679ba7e

Merge branch 'pulse_string_parser' of github.com:DanPuzzuoli/qiskit-d…

ba4a35b

…ynamics into pulse_string_parser

renaming various folders, files, and functions

8ce25bd

Merge branch 'main' into pulse_string_parser

9591fd9

Merge branch 'main' into pulse_string_parser

9fb2c31

DanPuzzuoli closed this Nov 22, 2022

DanPuzzuoli mentioned this pull request Nov 22, 2022

Implement DynamicsBackend.from_backend #154

Closed

DanPuzzuoli mentioned this pull request Dec 6, 2022

Implement DynamicsBackend.from_backend #157

Merged

		from .operator_from_string import operator_from_string, apply_func


		def legacy_parser(operator_str, subsystem_dims, subsystem_list):


		Token = namedtuple("Token", ("type", "name"))

		str_elements = OrderedDict(

		__funcdict = {"dag": dag}


		def apply_func(name: str, op: np.ndarray) -> np.ndarray:

		return np.diag(np.sqrt(np.arange(1, dim, dtype=complex)), 1)


		def adag(dim: int) -> np.ndarray:

		return static_hamiltonian, list(hamiltonian_operators), list(reduced_channels), subsystem_dims


		def hamiltonian_pre_parse_exceptions(hamiltonian_dict: dict):

	def hamiltonian_pre_parse_exceptions(hamiltonian_dict: dict):
	def _hamiltonian_pre_parse_exceptions(hamiltonian_dict: dict):

Conversation

DanPuzzuoli commented Dec 10, 2021

Summary

Details and comments

Uh oh!

brosand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanPuzzuoli commented Feb 8, 2022

Uh oh!

brosand commented Feb 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brosand left a comment

Choose a reason for hiding this comment

Uh oh!

chriseclectic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanPuzzuoli commented Feb 11, 2022

Uh oh!

DanPuzzuoli commented Nov 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects