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
232 stars 90 forks source link

Incorrect pitch for tips up sub-block pin grid #1284

Closed drewj-usnctech closed 7 months ago

drewj-usnctech commented 1 year ago

In the same vein of #1278, the pin pitch seems to be off. The following grid definitions

  pins:
    lattice pitch:
      x: 1.2
      y: 1.2
    geom: hex_corners_up
    lattice map: |
      - - O F F
       - F F F F
        F F F F F
         F F F F
          F F F
    # geom: hex
    # lattice map: |
    #   - F
    #    F F
    #   F F F
    #    F F
    #   F F F
    #    F F
    #   F F F
    #    F F
    #     F 

produce two different pitches. For the normal "flats up" hexagon, the pitch is correct: b.spatialGrid.pitch=1.2. But for the "tips up" grid, the pitch is off: b.spatialGrid.pitch=1.0392304845413265 using

import armi
armi.configure()
o = armi.init(fName="sample.yaml")
b = o.r.core[0][0]
g = b.spatialGrid
print(f"{b.spatialGrid.pitch=}")

My current workaround is to look at the _unitSteps attribute, which feels not ideal (using private attribute)

>>> b.spatialGrid._unitSteps
array([[ 0.6       , -0.6       ,  0.        ],
       [ 1.03923048,  1.03923048,  0.        ],
       [ 0.        ,  0.        ,  0.        ]])
>>> realPitch = b.spatialGrid._unitSteps[0,0] * 2

If I understand these right, the _unitSteps[0, 0] is the change in x for changing ring. So each new ring nets us a full new x-site, since they're all neighbors.

drewj-usnctech commented 1 year ago

In digging around to find out if this is a bug or not, I was reminded of https://github.com/terrapower/armi/issues/252 where the "undocumented" way to define a pitch for a hexagonal sub-assembly lattice is to use the x/y pitches

lattice pitch:
  x: 1.2

which still feels weird. Would it be worth making a separate issue for supporting a scalar for "regular" hex or cartesian grids?

lattice pitch: 1.2

feels a lot easier to explain to new users

keckler commented 1 year ago

Thank you for posting this. I think that this is clearly a bug, and needs to be fixed. As you can probably tell, this orientation of hex grid is not used as much, so we appreciate the bug reports.

I do not know of any reason that such a scalar lattice pitch could not be supported, and I would like to see it implemented.

drewj-usnctech commented 1 year ago

Bump on this nice to have feature

john-science commented 1 year ago

Order of Operations:

  1. PR 1373 goes through.
  2. Modify HexGrid to be two subclasses: Tips Up or Flats Up (or add an attribute in the class, or something)
  3. Modify these method to solve this ticket.
john-science commented 8 months ago

I think there may be a little confusion here.

The distances defined by x and y here are the "flat-to-flat" distance, whether the HexGrid is flat-to-flat or "corners up":

https://github.com/terrapower/armi/blob/6163e942abe873129533b84a6f2efa632f260b47/armi/reactor/grids/hexagonal.py#L110-L111

This exactly explains your numerical problem above:

1.0392304845413265 = math.sqrt(3)/2 * 1.2

And that factor (math.sqrt(3)/2), is the ratio of the "flat-to-flat" diameter of a hexagon over the "corner-to-corner" diameter.

I think this is not a bug, but just a misunderstanding. I would normally just close this ticket, but perhaps @drewj-usnctech could tell me where better to put this information so it would have been visible to him from the get-go? I don't want other people to run into the same problem, of course. And that docstring is apparently too hidden.

I do think it's awkward that the "lattice map" has "x" and "y", but for HexGrids of all kinds we only use or need "x".

mgjarrett commented 8 months ago

The actual pitch of the grid that is formed by b.spatialGrid._unitSteps in this case is correctly 1.2

But, the property b.spatialGrid.pitch is defined as _unitSteps[1][1] = 1.03923, which is not the actual pitch.

https://github.com/terrapower/armi/blob/6163e942abe873129533b84a6f2efa632f260b47/armi/reactor/grids/hexagonal.py#L152-L161

That definition is a shortcut that only works for flats up. But it is easy enough to calculate the pitch for any orientation of a hex block:

image