Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add option to colour wires in diagrams with frames #177

Merged
merged 44 commits into from
Nov 22, 2024
Merged

Add option to colour wires in diagrams with frames #177

merged 44 commits into from
Nov 22, 2024

Conversation

Ragunath1729
Copy link
Contributor

@Ragunath1729 Ragunath1729 commented Nov 7, 2024 •
edited
Loading

Now we can draw wires with colours. the diagram has frames will be rendered with colours by default,

  1. Wire colours be turned off using flag color_wires=False.
  2. Change the thickness by passing a float value as wires_width=1.5 in draw(). by default it uses 1.25 units
  3. Mat_backend and Tikz_backend colouring changes

Code

Mat_Backend

reader = DisCoCircReader()
reader.text2circuit(
    "Thunder dere 's a bump . Vy don'd you drive unk Sharlie . Drive yourself if you think you can do any better .").draw(
    figsize=(12, 8), foliated=True, wires_width=1.5, color_wires=True)

Tikz_backend

reader.text2circuit("Thunder dere 's a bump . Vy don'd you drive unk Sharlie . Drive yourself if you think you can do any better .").draw( foliated=False, to_tikz=True, color_boxes=True, color_wires=True, use_tikzstyles=True)

Screenshots

Screenshot 2024-11-12 at 13 44 13 Screenshot 2024-11-12 at 13 45 05 Screenshot 2024-11-12 at 13 46 02 Screenshot 2024-11-12 at 13 46 50 Screenshot 2024-11-12 at 13 47 53 Screenshot 2024-11-12 at 13 51 42

Tikz drawing

Screenshot 2024-11-14 at 21 17 45 Screenshot 2024-11-14 at 21 22 15

@Ragunath1729
Color wires changes. wires are coloured used by default when the diag…
14b8f85 
…ram has frames but can be turned off using flag `color_wires=False`
@dimkart dimkart requested a review from neiljdo November 7, 2024 13:13
@neiljdo
Copy link
Collaborator

neiljdo commented Nov 7, 2024

@Ragunath1729 a few questions/remarks:

  • The colored wires start with different colors in between diagrams - this is not intended, right?
  • The wires inside frames don't match the dom and cod wires of the frame - maybe it's better to leave them as black?

I'll create a more thorough review later.

neiljdo
neiljdo reviewed Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@neiljdo neiljdo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Ragunath1729, please take a look at the following comments. Also, please don't forget to do flake8 and mypy related fixes. And please add docstrings where necessary.

lambeq/backend/drawing/drawing_backend.py Outdated Show resolved Hide resolved
lambeq/backend/drawing/drawing.py Outdated Show resolved Hide resolved
lambeq/backend/drawing/drawing.py Outdated Show resolved Hide resolved
lambeq/backend/drawing/drawing.py Show resolved Hide resolved
lambeq/backend/drawing/drawing.py Outdated Show resolved Hide resolved
lambeq/backend/drawing/drawable.py Outdated Show resolved Hide resolved
@neiljdo neiljdo changed the title Changes for draw coloured wires in diagrams Add option to colour wires in diagrams with frames Nov 7, 2024
@Ragunath1729
Copy link
Contributor Author

Ragunath1729 commented Nov 11, 2024 •
edited
Loading

@Ragunath1729 a few questions/remarks:

@neiljdo responses below

The colored wires start with different colors in between diagrams - this is not intended, right? 

 yes, but now I have changed the noun_id to non-static variable where every time draw call to diagram.draw(), counter starts from zero.

The wires inside frames don't match the dom and cod wires of the frame - maybe it's better to leave them as black?

yes, now all wires inside the frames are drawn as black

I'll create a more thorough review later.

@dimkart
Copy link
Contributor

dimkart commented Nov 12, 2024

@Ragunath1729 @neiljdo In one of the last commits, Ragunath added thickness to the wires, which looks good if the diagram is not very big. It is worth however to check how it looks in the really big diagrams with many nouns, probably not that good. Perhaps we should try to reduce a bit the thickness, keeping the wires thicker than before though?

neiljdo
neiljdo reviewed Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@neiljdo neiljdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ragunath1729 I've left some comments, please take a look.

lambeq/backend/drawing/tikz_backend.py Outdated Show resolved Hide resolved
tests/backend/simple-books-frame-drawing.html Outdated Show resolved Hide resolved
lambeq/backend/drawing/drawing.py Outdated Show resolved Hide resolved
lambeq/backend/drawing/drawing_backend.py Outdated Show resolved Hide resolved
lambeq/backend/drawing/drawing_backend.py Outdated Show resolved Hide resolved
lambeq/backend/drawing/tikz_backend.py Outdated Show resolved Hide resolved
@Ragunath1729
Copy link
Contributor Author

@Ragunath1729 I've left some comments, please take a look.
@neiljdo , I have updated the code as per the review comments. kindly review again

neiljdo
neiljdo approved these changes Nov 22, 2024
View reviewed changes
Copy link
Collaborator

@neiljdo neiljdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@neiljdo neiljdo merged commit 9ce7774 into CQCL:main Nov 22, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljdo neiljdo neiljdo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ragunath1729 @neiljdo @dimkart