new: define (de)serializers for chip specs by notmgsk · Pull Request #27 · rigetti/libquil-sys

notmgsk · 2023-08-09T13:34:42Z

This MR adds crate::chip::ChipSpec (and related types) which provide a Quilc-compatible definition of chip specifications.

There is a corpus of data in ./test-data used for parity testing (deserialize -> serialize -> compare with original). It has been copied from the Quilc repository with small edits to fix JSON syntax and to change the types of some data (e.g. replace "true" with true).

BatmanAoD

Minor questions/suggestions

BatmanAoD · 2023-08-18T19:55:32Z

src/chip/isa.rs

+    #[serde(rename(deserialize = "1Q"))]
+    #[serde(rename(serialize = "1Q"))]


Style: throughout, why not rename these together?

Suggested change

#[serde(rename(deserialize = "1Q"))]

#[serde(rename(serialize = "1Q"))]

#[serde(rename = "1Q"))]

BatmanAoD · 2023-08-18T20:17:42Z

src/chip/isa.rs

+}
+
+#[derive(Debug, serde::Serialize, serde::Deserialize, PartialEq)]
+pub struct Metadata {}


Should this include a flattened map of miscellaneous fields, so that it's round-trip-able?

https://serde.rs/attr-flatten.html#capture-additional-fields

BatmanAoD · 2023-08-18T20:18:14Z

src/chip/isa.rs

+        #[serde(rename(deserialize = "type"))]
+        #[serde(rename(serialize = "type"))]
+        ty: String,


Style: why not just make the field-name type?

BatmanAoD · 2023-08-18T20:50:08Z

src/chip/isa.rs

+    #[serde(deserialize_with = "deserialize_wildcard")]
+    #[serde(serialize_with = "serialize_wildcard")]
+    Wildcard,


Would Wildcard(monostate::MustBe!("_")) work here?

Shadow53 · 2023-08-18T22:50:39Z

src/chip/isa.rs

+    #[serde(rename(deserialize = "1Q"))]
+    #[serde(rename(serialize = "1Q"))]


Suggested change

#[serde(rename(deserialize = "1Q"))]

#[serde(rename(serialize = "1Q"))]

#[serde(rename = "1Q")]

Shadow53 · 2023-08-18T22:51:00Z

src/chip/isa.rs

+    #[serde(rename(deserialize = "2Q"))]
+    #[serde(rename(serialize = "2Q"))]


Suggested change

#[serde(rename(deserialize = "2Q"))]

#[serde(rename(serialize = "2Q"))]

#[serde(rename = "2Q")]

Shadow53 · 2023-08-18T22:51:58Z

src/chip/isa.rs

+#[derive(Debug, serde::Serialize, serde::Deserialize, PartialEq)]
+pub struct Metadata {}


What is the purpose of this struct?

Shadow53 · 2023-08-18T22:52:49Z

src/chip/isa.rs

+#[serde(rename(serialize = "1Q"))]
+#[serde(rename_all = "lowercase")]
+#[serde(untagged)]
+pub enum TwoQ {


Suggested change

#[serde(rename(serialize = "1Q"))]

#[serde(rename_all = "lowercase")]

#[serde(untagged)]

pub enum TwoQ {

#[serde(rename(serialize = "2Q"))]

#[serde(rename_all = "lowercase")]

#[serde(untagged)]

pub enum TwoQ {

Shadow53 · 2023-08-18T22:53:09Z

src/chip/isa.rs

+        #[serde(rename(deserialize = "type"))]
+        #[serde(rename(serialize = "type"))]


Suggested change

#[serde(rename(deserialize = "type"))]

#[serde(rename(serialize = "type"))]

#[serde(rename = "type")]

Shadow53 · 2023-08-18T22:57:10Z

src/chip/isa.rs

+#[serde(untagged)]
+pub enum Gate {
+    Measure {
+        operator: monostate::MustBe!("MEASURE"),
+        qubit: Qubit,
+        target: Option<MeasurementTarget>,
+        duration: f64,
+        fidelity: f64,
+    },
+    Quantum {
+        operator: String,
+        parameters: Vec<Parameter>,
+        arguments: Vec<Argument>,
+        duration: f64,
+        fidelity: f64,
+    },
+}


I think this does the same thing and does not require the extra crate.

Suggested change

#[serde(untagged)]

pub enum Gate {

Measure {

operator: monostate::MustBe!("MEASURE"),

qubit: Qubit,

target: Option<MeasurementTarget>,

duration: f64,

fidelity: f64,

},

Quantum {

operator: String,

parameters: Vec<Parameter>,

arguments: Vec<Argument>,

duration: f64,

fidelity: f64,

},

}

#[serde(tag = "operator")]

pub enum Gate {

#[serde(rename = "MEASURE")]

Measure {

qubit: Qubit,

target: Option<MeasurementTarget>,

duration: f64,

fidelity: f64,

},

#[serde(other)]

Quantum {

operator: String,

parameters: Vec<Parameter>,

arguments: Vec<Argument>,

duration: f64,

fidelity: f64,

},

}

Shadow53 · 2023-08-18T22:59:05Z

src/chip/isa.rs

+    #[serde(deserialize_with = "deserialize_wildcard")]
+    #[serde(serialize_with = "serialize_wildcard")]


Would this do the same thing and be simpler?

Suggested change

#[serde(deserialize_with = "deserialize_wildcard")]

#[serde(serialize_with = "serialize_wildcard")]

#[serde(rename = "_")]

(Same for all the other cases as well)

Doesn't rename change the key name, not the value?

Shadow53 · 2023-08-18T22:59:45Z

src/chip/isa.rs

+    use serde_json::json;
+
+    #[test]
+    fn deserialize_oneq() {


Don't see a test for 2Q

Shadow53 · 2023-08-18T23:02:42Z

src/chip/mod.rs

+    pub timestamp: Option<String>,
+    #[serde(deserialize_with = "deserialize_string_from_number")]
+    #[serde(serialize_with = "maybe_serialize_string_to_integer")]
+    pub version: String,


Seems like it might be better to have a Version struct instead of the manual serialization? Would also mean not needing the extra crate.

#[derive(Debug, serde::Serialize, serde::Deserialize)] #[serde(untagged)] pub enum Version { Number(i32), String(String), }

Shadow53 · 2023-08-18T23:04:47Z

src/chip/specs.rs

+#[derive(Debug, serde::Serialize, serde::Deserialize, PartialEq)]
+pub struct SpecsMap(HashMap<String, f64>);


Is there a specific reason for using a newtype pattern here instead of a type alias? Why not this?

Suggested change

#[derive(Debug, serde::Serialize, serde::Deserialize, PartialEq)]

pub struct SpecsMap(HashMap<String, f64>);

pub type SpecsMap = HashMap<String, f64>;

notmgsk added 2 commits August 9, 2023 14:31

new: define (de)serializers for chip specs

9c7f686

Add more docstrs

6cd2ebb

notmgsk marked this pull request as ready for review August 9, 2023 13:49

notmgsk requested review from BatmanAoD, MarquessV, Shadow53 and kalzoo August 9, 2023 19:50

BatmanAoD approved these changes Aug 18, 2023

View reviewed changes

Shadow53 requested changes Aug 18, 2023

View reviewed changes

notmgsk force-pushed the main branch 2 times, most recently from 9b564fa to 586eaeb Compare October 16, 2023 16:32

		#[serde(rename(deserialize = "1Q"))]
		#[serde(rename(serialize = "1Q"))]

	#[serde(rename(deserialize = "1Q"))]
	#[serde(rename(serialize = "1Q"))]
	#[serde(rename = "1Q"))]

	#[serde(rename(deserialize = "1Q"))]
	#[serde(rename(serialize = "1Q"))]
	#[serde(rename = "1Q")]

		#[serde(rename(deserialize = "2Q"))]
		#[serde(rename(serialize = "2Q"))]

	#[serde(rename(deserialize = "2Q"))]
	#[serde(rename(serialize = "2Q"))]
	#[serde(rename = "2Q")]

		#[derive(Debug, serde::Serialize, serde::Deserialize, PartialEq)]
		pub struct Metadata {}

		#[serde(rename(deserialize = "type"))]
		#[serde(rename(serialize = "type"))]

	#[serde(rename(deserialize = "type"))]
	#[serde(rename(serialize = "type"))]
	#[serde(rename = "type")]

		#[serde(deserialize_with = "deserialize_wildcard")]
		#[serde(serialize_with = "serialize_wildcard")]

	#[serde(deserialize_with = "deserialize_wildcard")]
	#[serde(serialize_with = "serialize_wildcard")]
	#[serde(rename = "_")]

		#[derive(Debug, serde::Serialize, serde::Deserialize, PartialEq)]
		pub struct SpecsMap(HashMap<String, f64>);

	#[derive(Debug, serde::Serialize, serde::Deserialize, PartialEq)]
	pub struct SpecsMap(HashMap<String, f64>);
	pub type SpecsMap = HashMap<String, f64>;

Conversation

notmgsk commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BatmanAoD 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

notmgsk commented Aug 9, 2023 •

edited

Loading