Update displayed device specs #763

HaroldErbin · 2024-11-19T11:28:05Z

Add more information to _specs() and move the method to BaseDevice.

A public property specs has been added.

See issue #756.

In this way, _specs and print_specs can also be called for virtual devices.

HaroldErbin · 2024-11-19T12:42:33Z

I don't understand how to make the typing test pass. If I use a try: ... except AttributeError to see if ch has an attribute bottom_detuning, then I get an error saying that ch has no attribute bottom_detuning (which is what I am testing. If I am using a different test with hasattr, then I get an error saying that there is a redundant cast to float`. Any pointer to solve this would be appreciated.

pulser-core/pulser/devices/_device_datacls.py

HaroldErbin · 2024-11-19T14:50:14Z

mypy continues to fail, but in a file I did not modify. Should I try to fix it nonetheless?

HGSilveri · 2024-11-19T14:59:55Z

mypy continues to fail, but in a file I did not modify. Should I try to fix it nonetheless?

Yeah, I saw this pop-up too in another PR. Please just copy this line and it goes away:

Pulser/pulser-pasqal/pulser_pasqal/pasqal_cloud.py

Line 241 in 67197da

assert isinstance(batch.sequence_builder, str)

HaroldErbin · 2024-11-19T15:40:51Z

Concerning the test coverage, which is now needed for specs since it's not internal, I was wondering what's the best approach. The simplest way is to assert that the specs string of different devices matches what is expected. However, this means having many lines of text (since we have to split the lines to respect the code style). Do you see a simpler way?

HGSilveri · 2024-11-19T16:43:45Z

Concerning the test coverage, which is now needed for specs since it's not internal, I was wondering what's the best approach. The simplest way is to assert that the specs string of different devices matches what is expected. However, this means having many lines of text (since we have to split the lines to respect the code style). Do you see a simpler way?

I'm afraid I don't see a way around it, no. I would maybe use a string with some format fields so that you can change some parameters (using @pytest.mark.parametrize) and check that the string always matches what you expect.

HaroldErbin · 2024-11-20T12:03:02Z

I have added a test for three devices. At the end, I did not use pytest.mark.parametrize but a function, let me know if this works for you.

HaroldErbin · 2024-11-20T12:04:39Z

To give an example of what has been implemented in the PR, here is the output of AnalogDevice._specs() before:

Register parameters:
 - Dimensions: 2D
 - Rydberg level: 60
 - Maximum number of atoms: 25
 - Maximum distance from origin: 35 μm
 - Minimum distance between neighbouring atoms: 5 μm
 - Maximum layout filling fraction: 0.5
 - SLM Mask: No
 - Maximum sequence duration: 4000 ns

Channels:
 - 'rydberg_global': Rydberg.Global(Max Absolute Detuning: 125.66370614359172 rad/µs, Max Amplitude: 12.566370614359172 rad/µs, Clock period: 4 ns, Minimum pulse duration: 16 ns, Maximum pulse duration: 100000000 ns, Modulation Bandwidth: 8 MHz, Basis: 'ground-rydberg')

and after:

Register parameters:
 - Dimensions: 2D
 - Rydberg level: 60
 - Maximum number of atoms: 25
 - Maximum distance from origin: 35 μm
 - Minimum distance between neighbouring atoms: 5 μm
 - Maximum layout filling fraction: 0.5
 - SLM Mask: No
 - Maximum sequence duration: 4000 ns

Device parameters:
 - Maximum number of runs: 2000
 - Channels can be reused: No
 - Supported bases: ground-rydberg
 - Supported states: r, g
 - Ising interaction coefficient: 865723.02

Layout parameters:
 - Requires layout: Yes
 - Accepts new layout: No
 - Minimal number of traps: 1
 - Maximal number of traps: None

Channels:
 - 'rydberg_global': Rydberg.Global(Max Absolute Detuning: 125.66370614359172 rad/µs, Max Amplitude: 12.566370614359172 rad/µs, Clock period: 4 ns, Minimum pulse duration: 16 ns, Maximum pulse duration: 100000000 ns, Modulation Bandwidth: 8 MHz, Basis: 'ground-rydberg')

Also, we can invoke this method for MockDevice now:

Register parameters:
 - Dimensions: 3D
 - Rydberg level: 70
 - Maximum number of atoms: None
 - Maximum distance from origin: None μm
 - Minimum distance between neighbouring atoms: 0.0 μm
 - Maximum layout filling fraction: 0.5
 - SLM Mask: Yes

Device parameters:
 - Channels can be reused: Yes
 - Supported bases: XY, ground-rydberg, digital
 - Supported states: u, d, r, g, h
 - Ising interaction coefficient: 5420158.53
 - XY interaction coefficient: 3700.0

Layout parameters:
 - Requires layout: No
 - Minimal number of traps: 1
 - Maximal number of traps: None

Channels:
 - 'rydberg_global': Rydberg.Global(Max Absolute Detuning: None, Max Amplitude: None, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'ground-rydberg')
 - 'rydberg_local': Rydberg.Local(Max Absolute Detuning: None, Max Amplitude: None, Minimum retarget time: 0 ns, Fixed retarget time: 0 ns, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'ground-rydberg')
 - 'raman_global': Raman.Global(Max Absolute Detuning: None, Max Amplitude: None, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'digital')
 - 'raman_local': Raman.Local(Max Absolute Detuning: None, Max Amplitude: None, Minimum retarget time: 0 ns, Fixed retarget time: 0 ns, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'digital')
 - 'mw_global': Microwave.Global(Max Absolute Detuning: None, Max Amplitude: None, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'XY')
 - 'dmm_0': DMM.Global(Max Absolute Detuning: None, Max Amplitude: 0, Clock period: 1 ns, Minimum pulse duration: 1 ns, Maximum pulse duration: 100000000 ns, Basis: 'ground-rydberg')

HGSilveri

Thank you for this @HaroldErbin , very nice work! I have some questions and suggestions, I'd be happy to hear your thoughts.

pulser-core/pulser/devices/_device_datacls.py

tests/test_devices.py

pulser-core/pulser/devices/_device_datacls.py

One change is to use a string instead of joining elements of a list to get the final string. The reason is that lists were cumbersome to use when there were conditional statements.

HaroldErbin · 2024-11-20T19:57:12Z

I have updated the codes with your suggestions, leaving only two conversations open for further discussions.

HGSilveri

Thanks for the changes! Can you please share a print of how this is looking right now?

pulser-core/pulser/devices/_device_datacls.py

Create one _specs method for each sections (register, layout, device, channels). The layout section is defined only in Device, such that it is not displayed for VirtualDevice. This commit also goes back to using lists for storing the lines.

HaroldErbin · 2024-11-21T18:59:17Z

I have implemented the changes suggested by @HGSilveri:

Individual private methods have been created for each section (register, layout, device, channels).
The layout section is defined only for Device.
I have reverted back to using lists instead of strings.
I have defined two new private methods which are used to improve handling of None and boolean parameters.

Here are the results:

DigitalAnalogDevice

Register parameters:
 - Dimensions: 2D
 - Rydberg level: 70
 - Maximum number of atoms: 100
 - Maximum distance from origin: 50 µm
 - Minimum distance between neighbouring atoms: 4 μm
 - SLM Mask: Yes

Layout parameters:
 - Requires layout: Yes
 - Accepts new layout: Yes
 - Minimal number of traps: 1
 - Maximum layout filling fraction: 0.5

Device parameters:
 - Channels can be reused: No
 - Supported bases: digital, ground-rydberg
 - Supported states: r, g, h
 - Ising interaction coefficient: 5420158.53

Channels:
 - 'rydberg_global': Rydberg.Global(Max Absolute Detuning: 125.66370614359172 rad/µs, Max Amplitude: 15.707963267948966 rad/µs, Clock period: 4 ns, Minimum pulse duration: 16 ns, Maximum pulse duration: 67108864 ns, Basis: 'ground-rydberg')
 - 'rydberg_local': Rydberg.Local(Max Absolute Detuning: 125.66370614359172 rad/µs, Max Amplitude: 62.83185307179586 rad/µs, Minimum retarget time: 220 ns, Fixed retarget time: 0 ns, Max targets: 1, Clock period: 4 ns, Minimum pulse duration: 16 ns, Maximum pulse duration: 67108864 ns, Basis: 'ground-rydberg')
 - 'raman_local': Raman.Local(Max Absolute Detuning: 125.66370614359172 rad/µs, Max Amplitude: 62.83185307179586 rad/µs, Minimum retarget time: 220 ns, Fixed retarget time: 0 ns, Max targets: 1, Clock period: 4 ns, Minimum pulse duration: 16 ns, Maximum pulse duration: 67108864 ns, Basis: 'digital')
 - 'dmm_0': DMM.Global(Max Absolute Detuning: None, Max Amplitude: 0, Clock period: 4 ns, Minimum pulse duration: 16 ns, Maximum pulse duration: 67108864 ns, Basis: 'ground-rydberg')

MockDevice

Register parameters:
 - Dimensions: 3D
 - Rydberg level: 70
 - Minimum distance between neighbouring atoms: 0.0 μm
 - SLM Mask: Yes

Device parameters:
 - Channels can be reused: Yes
 - Supported bases: digital, ground-rydberg, XY
 - Supported states: u, d, r, g, h
 - Ising interaction coefficient: 5420158.53
 - XY interaction coefficient: 3700.0

Channels:
 - 'rydberg_global': Rydberg.Global(Max Absolute Detuning: None, Max Amplitude: None, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'ground-rydberg')
 - 'rydberg_local': Rydberg.Local(Max Absolute Detuning: None, Max Amplitude: None, Minimum retarget time: 0 ns, Fixed retarget time: 0 ns, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'ground-rydberg')
 - 'raman_global': Raman.Global(Max Absolute Detuning: None, Max Amplitude: None, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'digital')
 - 'raman_local': Raman.Local(Max Absolute Detuning: None, Max Amplitude: None, Minimum retarget time: 0 ns, Fixed retarget time: 0 ns, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'digital')
 - 'mw_global': Microwave.Global(Max Absolute Detuning: None, Max Amplitude: None, Clock period: 1 ns, Minimum pulse duration: 1 ns, Basis: 'XY')
 - 'dmm_0': DMM.Global(Max Absolute Detuning: None, Max Amplitude: 0, Clock period: 1 ns, Minimum pulse duration: 1 ns, Maximum pulse duration: 100000000 ns, Basis: 'ground-rydberg')

HGSilveri

Super nice work @HaroldErbin , thanks a lot! I have a final proposition for the return types of these new methods that I think can make them slightly more flexible. Consider all the comments below together, they are related

HGSilveri · 2024-11-22T09:06:18Z

pulser-core/pulser/devices/_device_datacls.py

-                " - Maximum sequence duration: "
-                f"{self.max_sequence_duration} ns"
-            )
+        return "\n".join(line for line in register_lines if line != "")


Can we return list[str] instead? This (and the other similar methods) could change to

Suggested change

return "\n".join(line for line in register_lines if line != "")

return [line for line in register_lines if line != ""]

HGSilveri · 2024-11-22T09:15:02Z

pulser-core/pulser/devices/_device_datacls.py

+        return "\n".join(
+            [
+                self._register_lines(),
+                self._device_lines(),
+                self._channel_lines(for_docs=for_docs),
+            ]
+        )


This would then become

Suggested change

return "\n".join(

[

self._register_lines(),

self._device_lines(),

self._channel_lines(for_docs=for_docs),

]

)

return "\n".join(

self._register_lines()

+ self._device_lines()

+ self._channel_lines(for_docs=for_docs)

)

HGSilveri · 2024-11-22T09:20:25Z

pulser-core/pulser/devices/_device_datacls.py

+    def _layout_lines(self) -> str:
+
+        layout_lines = [
+            "\nLayout parameters:",
+            f" - Requires layout: {self._param_yes_no(self.requires_layout)}",
+            " - Accepts new layout: "
+            + self._param_yes_no(self.accepts_new_layouts),
+            f" - Minimal number of traps: {self.min_layout_traps}",
+            self._param_check_none(self.max_layout_traps)(
+                " - Maximal number of traps: {}"
+            ),
+            f" - Maximum layout filling fraction: {self.max_layout_filling}",
+        ]
+
+        return "\n".join(line for line in layout_lines if line != "")


I had something slightly different in mind for the layout parameters. I think the ones that are in VirtualDevice should also appear in its specs, so this method could be moved to BaseDevice expect for the accepts_new_layout line. That one, we could add it like this:

Suggested change

def _layout_lines(self) -> str:

layout_lines = [

"\nLayout parameters:",

f" - Requires layout: {self._param_yes_no(self.requires_layout)}",

" - Accepts new layout: "

+ self._param_yes_no(self.accepts_new_layouts),

f" - Minimal number of traps: {self.min_layout_traps}",

self._param_check_none(self.max_layout_traps)(

" - Maximal number of traps: {}"

),

f" - Maximum layout filling fraction: {self.max_layout_filling}",

]

return "\n".join(line for line in layout_lines if line != "")

def _layout_lines(self) -> list[str]:

layout_lines = super()._layout_lines()

layout_lines.insert(2, f" - Accepts new layout: {self._param_yes_no(self.accepts_new_layouts)}"

return layout_lines

HGSilveri · 2024-11-22T09:20:45Z

pulser-core/pulser/devices/_device_datacls.py

+    def _specs(self, for_docs: bool = False) -> str:
+
+        return "\n".join(
+            [
+                self._register_lines(),
+                self._layout_lines(),
+                self._device_lines(),
+                self._channel_lines(for_docs=for_docs),
+            ]
+        )


This (or rather its new version) would then be in BaseDevice and we don't need to redefine it here

HaroldErbin added 8 commits November 19, 2024 11:53

Add all missing specs

a8cd5f8

Add property specs

eaba991

Move _specs to BaseDevice

10c3a67

In this way, _specs and print_specs can also be called for virtual devices.

Fix syntax for compatibility with older Python

cf48c7a

Fix style

93278c8

Add missing docstring

e0691d5

Fix dosctring style

3138ad1

Improve specs method

892908c

HGSilveri reviewed Nov 19, 2024

View reviewed changes

pulser-core/pulser/devices/_device_datacls.py Outdated Show resolved Hide resolved

Update to fix mypy errors

d671753

Fix mypy error

62e9a09

Add tests for BaseDevice.specs property

a70e20d

Fix import order

d61c6cd

HGSilveri reviewed Nov 20, 2024

View reviewed changes

Various minor improvements

9bddf28

One change is to use a string instead of joining elements of a list to get the final string. The reason is that lists were cumbersome to use when there were conditional statements.

HGSilveri reviewed Nov 21, 2024

View reviewed changes

pulser-core/pulser/devices/_device_datacls.py Outdated Show resolved Hide resolved

HGSilveri changed the title ~~Specs~~ Update displayed device specs Nov 21, 2024

HGSilveri linked an issue Nov 21, 2024 that may be closed by this pull request

Show all the specs of the Device #756

Open

Split _specs method in different methods

e173206

Create one _specs method for each sections (register, layout, device, channels). The layout section is defined only in Device, such that it is not displayed for VirtualDevice. This commit also goes back to using lists for storing the lines.

HaroldErbin added 2 commits November 21, 2024 20:03

Remove line

6ce5a15

Fix typo in strings

846db9a

Merge remote-tracking branch 'refs/remotes/origin/specs' into specs

846721c

HGSilveri reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update displayed device specs #763

Update displayed device specs #763

HaroldErbin commented Nov 19, 2024

HaroldErbin commented Nov 19, 2024

HaroldErbin commented Nov 19, 2024

HGSilveri commented Nov 19, 2024

HaroldErbin commented Nov 19, 2024

HGSilveri commented Nov 19, 2024

HaroldErbin commented Nov 20, 2024

HaroldErbin commented Nov 20, 2024 •

edited

Loading

HGSilveri left a comment

HaroldErbin commented Nov 20, 2024

HGSilveri left a comment

HaroldErbin commented Nov 21, 2024

HGSilveri left a comment

HGSilveri Nov 22, 2024

HGSilveri Nov 22, 2024

HGSilveri Nov 22, 2024

HGSilveri Nov 22, 2024 •

edited

Loading

	return "\n".join(line for line in register_lines if line != "")
	return [line for line in register_lines if line != ""]

Update displayed device specs #763

Are you sure you want to change the base?

Update displayed device specs #763

Conversation

HaroldErbin commented Nov 19, 2024

HaroldErbin commented Nov 19, 2024

HaroldErbin commented Nov 19, 2024

HGSilveri commented Nov 19, 2024

HaroldErbin commented Nov 19, 2024

HGSilveri commented Nov 19, 2024

HaroldErbin commented Nov 20, 2024

HaroldErbin commented Nov 20, 2024 • edited Loading

HGSilveri left a comment

Choose a reason for hiding this comment

HaroldErbin commented Nov 20, 2024

HGSilveri left a comment

Choose a reason for hiding this comment

HaroldErbin commented Nov 21, 2024

HGSilveri left a comment

Choose a reason for hiding this comment

HGSilveri Nov 22, 2024

Choose a reason for hiding this comment

HGSilveri Nov 22, 2024

Choose a reason for hiding this comment

HGSilveri Nov 22, 2024

Choose a reason for hiding this comment

HGSilveri Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

HaroldErbin commented Nov 20, 2024 •

edited

Loading

HGSilveri Nov 22, 2024 •

edited

Loading