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

Add auto fading for off-diagonal blocks #50

Merged
merged 15 commits into from
Mar 21, 2023
Merged

Add auto fading for off-diagonal blocks #50

merged 15 commits into from
Mar 21, 2023

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Aug 30, 2022

Purpose

Basically, this is meant to make it easier to make a sequence of diagrams with different blocks faded. Instead of having to manually specify which off-diagonal components are faded, I've added some logic (with some options) to automatically decide if off-diagonal components connected to the faded diagonal components are to be faded.

This is still a fairly crude implementation, I'm happy to iterate on this PR to figure out a better API/implementation. Some initial thoughts:

  • have a finalize function that gets called when we write out the TeX file, to sort out the logic after everything's added properly
  • more logic to decide whether or not to shade connections if one or both sides are shaded

Expected time until merged

As long as it takes.

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Checklist

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@ewu63 ewu63 requested a review from a team as a code owner August 30, 2022 03:49
@ewu63 ewu63 requested review from marcomangano and A-CGray August 30, 2022 03:49
@lamkina
Copy link
Contributor

lamkina commented Aug 31, 2022

Something else to consider, it would be nice if the unfaded connections would come after the faded connections in the tikz file. Tiks works with layers so if faded connections are added after the un-faded connections they over-write the unfaded connections.

@ewu63
Copy link
Collaborator Author

ewu63 commented Sep 1, 2022

Something else to consider, it would be nice if the unfaded connections would come after the faded connections in the tikz file. Tiks works with layers so if faded connections are added after the un-faded connections they over-write the unfaded connections.

Yeah good point, this can also go into that finalize() function I mentioned.

@ewu63
Copy link
Collaborator Author

ewu63 commented Sep 27, 2022

I fixed the faded process stuff. Some of the concerns are still not addressed but I don't think I will continue to work on this, so I think either someone else should take over this PR or we should merge as is and iterate later on for further improvements. @lamkina

@lamkina
Copy link
Contributor

lamkina commented Sep 30, 2022

@nwu63 What are the remaining concerns? I can try and work on this if there's a demand for other features to be added/fixed.

@ewu63
Copy link
Collaborator Author

ewu63 commented Sep 30, 2022

I've addressed everything on my end. However the logic is not super clean so it's up to the reviewers whether they think this is okay. I also did not address your concern with the ordering of the layers but that could be dealt with separately I think.

pyxdsm/XDSM.py Outdated Show resolved Hide resolved
pyxdsm/XDSM.py Outdated Show resolved Hide resolved
pyxdsm/XDSM.py Outdated Show resolved Hide resolved
@eirikurj
Copy link
Contributor

Whats the status on this? Are there any remaining concerns or changes that need to done?

@marcomangano
Copy link
Contributor

I might remember incorrectly, but @lamkina wanted to make changes? Otw I can review this shortly

@lamkina
Copy link
Contributor

lamkina commented Feb 21, 2023

I might remember incorrectly, but @lamkina wanted to make changes? Otw I can review this shortly

I think the changes I want to make may be better served for PR #52 or a different PR. I would check with @A-CGray if the changes he requested are good to go or not.

@A-CGray
Copy link
Member

A-CGray commented Mar 6, 2023

I think this is done from my perspective now, I implemented the change I requested a while back.

The behaviour of the auto fading is still not perfect because there are often multiple connections/inputs/outputs drawn on top of each other. To be sure of getting the intended fading you probably need to do all the fading manually but the auto_fade is still a nice first option.

A-CGray
A-CGray previously approved these changes Mar 6, 2023
@A-CGray A-CGray requested a review from lamkina March 6, 2023 18:13
@A-CGray
Copy link
Member

A-CGray commented Mar 6, 2023

@lamkina or @marcomangano can you review my latest changes and then merge if you're happy

Copy link
Contributor

@lamkina lamkina left a comment

Choose a reason for hiding this comment

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

I added logic to fix fading the connections in the tikz figure so I think this is ready to merge now.

@A-CGray A-CGray merged commit a5eb8eb into main Mar 21, 2023
@A-CGray A-CGray deleted the add-auto-fade branch March 21, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants