-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fix rotation of hexagons in block diagrams #1648
base: main
Are you sure you want to change the base?
Conversation
Note: others may be working on parallel solutions to the issues linked in the PR description. I'm just trying to share the bare-bones outline of a path we could take to fix the problem; I'm not necessarily saying this PR needs to be merged. |
As I said in the other ticket (#1278), I have am working on a PR that fixes two different I like your property, and haven't looked at your plotting fix yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mgjarrett for trying these changes! I think there are a couple things that need to be fixed before we can maybe merge it.
Ultimately I think we can make it work, but man this plotBlockDiagram
method is frustratingly specific and fragile. In the longer term, it might make sense to split the method into two methods to handle the cases of hexagonal vs cartesian assemblies independently. I think that would make things a lot cleaner. Not asking for you to do this now, just maybe something to think about in a later ticket.
I might also suggest changing the name of the PR, since this PR is mostly about fixing the block diagrams, IMO. #1649 includes the same changes related to the new |
@keckler thanks for the review. My goal here was just to show in code what the outline of my idea for fixing the problem was; I thought it would be easier than describing it in words. I knew @john-science was working on this issue, so I wanted to wait and see what the code looked like after he finished his work. Now that he's fixed most of the confusion between flats/corners up, I think it makes sense to change the scope of this to just fixing |
@keckler I incorporated your comments; the plots look good now (at least for a flats up block; I don't have a corners up block handy for testing). I merged #1649 into this branch locally and tested it, and the plotting still works. So once #1649 is merged into main, this PR can be merged with main and it should all work out. |
I thought about adding unit tests, but this is mostly plotting stuff that is difficult to test directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
Co-authored-by: Chris Keckler <[email protected]>
I looked into this problem a little more. There's nothing wrong with the plotting code necessarily, it's just that Maybe we could check the orientation of the pin coordinates once the block has been placed in a |
What is the change?
Rotate hex patches based on the
cornersUp
property before plotting for a block diagram.Why is the change being made?
The rotation was previously hard-coded to assume a "flats up" hexagon block. This is not always the case. A new
cornersUp
property for theHexGrid
class implemented in #1649 allows us to check for the correct orientation of the hexagons.plotBlockDiagram()
doesn't respect block orientation #1421Checklist
doc/release/0.X.rst
) are up-to-date with any important changes.doc
folder.pyproject.toml
.