Draw layout and register on the same plot #772
Conversation
There was a problem hiding this comment.
Thanks for your PR @BrFle !
It already looks quite good ! I have a suggestion regarding the display of the empty traps. Let me know what you think about it.
As per the testing. It is really not easy to test these kind of function with pytest, since we can't see the display. You will have to add a test to tests/test_register.py, where you would do reg.draw(draw_empty_sites=True) (and that it raises an error with a register not having a layout associated to it).
I suggest that you add draw_empty_sites=True in the two examples on "Register Definition" in the tutorial on Register Layout: https://pulser.readthedocs.io/en/stable/tutorials/reg_layouts.html#Register-definition
| @@ -404,6 +404,8 @@ def draw( | |||
| kwargs_savefig: dict = {}, | |||
| custom_ax: Optional[Axes] = None, | |||
| show: bool = True, | |||
| draw_empty_sites: bool = False, | |||
| empty_color: Optional[str] = None, | |||
There was a problem hiding this comment.
May I suggest to have the default color instead of None ? I also suggest we have the default color to be gray instead of red, to be consistent with the current color of a layout ?
Another idea I had, which might make the life of the user easier, would be to have the empty sites displayed as a trap in the layout, a black circle filled with white. What do you think of this alternative ? An advantage I see with this is that there will be no conflict with qubit_colors (in the current implementation, what happens if the user provide qubit_colors=empty_color ? The visualization will not be great...)
| empty_color: Optional[str] = None, | |
| empty_color: str = "gray", |
| @@ -434,6 +436,8 @@ def draw( | |||
| show: Whether or not to call `plt.show()` before returning. When | |||
| combining this plot with other ones in a single figure, one may | |||
| need to set this flag to False. | |||
| draw_empty_sites: If True, draws the empty sites as well. | |||
| empty_color: The color of the empty sites. Default is 'r'. | |||
There was a problem hiding this comment.
As in my comment above, I am wondering if empty_color is necessary, and I would perhaps suggest to have default to "gray" instead of "r".
| empty_layout = self.layout.define_register( | ||
| *layout_ids, qubit_ids=layout_ids | ||
| ) | ||
| breakpoint() |
There was a problem hiding this comment.
I think we should not leave this breakpoint() in the code :)
| ) | ||
|
|
||
| if draw_empty_sites: | ||
| RegDrawer._draw_2D(empty_layout, |
There was a problem hiding this comment.
I think you can use super instead of RegDrawer :)
| if draw_empty_sites: | ||
| RegDrawer._draw_2D(empty_layout, | ||
| ids=empty_layout._ids, | ||
| pos=empty_pos, |
There was a problem hiding this comment.
Your implementation looks good. It enables you to implement the alternative I have presented above, where the empty spots are circles filled with white. To implement it fully, you might want to take out the positions of the atoms in the Register in empty_pos, because at the moment you are super-imposing two symbols for these filled positions and the symbol of the circle with white inside is larger than the symbol for the atoms (a disk of green by default, or a color defined in qubit_colors).
|
If you want to draw the empty sites as traps in a RegisterLayout, you should specify them as traps in the RegisterDrawer._draw_2D, by saying |
HGSilveri
changed the title
|
Hey @BrFle, is everything going well with this PR ? 😄 |
First draft to add option to draw the layout.
Still missing tests !
Fixes #726