terrapower / armi

An open-source nuclear reactor analysis automation framework that helps design teams increase efficiency and quality
https://terrapower.github.io/armi/
Apache License 2.0
224 stars 87 forks source link

Fix rotation of hexagons in block diagrams #1648

Open mgjarrett opened 7 months ago

mgjarrett commented 7 months ago

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 the HexGrid class implemented in #1649 allows us to check for the correct orientation of the hexagons.

Checklist

mgjarrett commented 7 months ago

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.

john-science commented 7 months ago

As I said in the other ticket (https://github.com/terrapower/armi/issues/1278), I have am working on a PR that fixes two different HexGrid bugs. I "assigned" that ticket to me to reduce duplicating work. Sorry about that.

I like your property, and haven't looked at your plotting fix yet.

keckler commented 7 months ago

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 cornersUp property.

mgjarrett commented 7 months ago

@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 plotBlockDiagram, as you suggest. I'll take a stab at it when I get a chance today.

mgjarrett commented 7 months ago

@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).

image

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.

mgjarrett commented 7 months ago

I thought about adding unit tests, but this is mostly plotting stuff that is difficult to test directly.

mgjarrett commented 7 months ago

Hmm still needs some work for corners up. I think the plotting is right, but the generation of the pin grid is wrong.

image

mgjarrett commented 6 months ago

Hmm still needs some work for corners up. I think the plotting is right, but the generation of the pin grid is wrong.

image

I looked into this problem a little more. There's nothing wrong with the plotting code necessarily, it's just that block.autoCreateSpatialGrids() only works if the block is "flats up". I tried to figure out a way to make it work with cornersUp blocks, but the block doesn't know what kind of grid it is going into when the local pin coordinates are auto-created.

Maybe we could check the orientation of the pin coordinates once the block has been placed in a core grid, and then rotate the pins 30 degrees counterclockwise if necessary? It's not a great solution, but it's the only thing I can think of.