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.

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

Updated "generate_diagram.py" script #1384

Merged

Conversation

apdavison
Copy link
Member

based on changes by Julia Sprenger in #1026

@zm711
Copy link
Contributor

zm711 commented Jan 29, 2024

Just one tiny thing to think about for the future. The color combinations aren't super friendly to people with color-blindness (green+red+blue-green-ish). Since the diagram is already a little tricky to wrap your head around we may want to watch out for this aqua vs red since they are currently conveying parts of the hierarchy.

@apdavison
Copy link
Member Author

Thanks for the comment @zm711

I checked out the figure at https://www.color-blindness.com/coblis-color-blindness-simulator/ and I think that even for red-blindness the colours are distinct, but I'm happy to take suggestions for different colours.

@zm711
Copy link
Contributor

zm711 commented Jan 29, 2024

As long as you check then it's probably good. I just like making sure that someone has checked. :)

@zm711
Copy link
Contributor

zm711 commented Jan 29, 2024

I'm still figuring out where all the figures live in the repo, but this figure also is outdated no (since it has groups containing multiple segments?)
image

@apdavison
Copy link
Member Author

apdavison commented Jan 29, 2024

this figure also is outdated no (since it has groups containing multiple segments?)

no. It is not a problem for a group to contain objects that belong to different segments, in fact that's one of the main use cases for Group.

(Groups can no longer contain entire Segment objects, but that is not shown in the figure)

@zm711
Copy link
Contributor

zm711 commented Jan 29, 2024

Cool. So it would be incorrect if the the orange box was over one or both black boxes. I see. Thanks @apdavison !

@samuelgarcia
Copy link
Contributor

Merci Andrew and Julia for this Heoric patch on this very old and dirty diagram generation code.

@alejoe91 alejoe91 merged commit 288e101 into NeuralEnsemble:master Feb 2, 2024
1 check passed
@apdavison apdavison deleted the update-relationship-diagram branch February 2, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants