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

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

Closed
drewj-usnctech opened this issue May 30, 2023 · 6 comments · Fixed by #1649
Closed

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

drewj-usnctech opened this issue May 30, 2023 · 6 comments · Fixed by #1649
Assignees
Labels
bug Something is wrong: Highest Priority

Comments

@drewj-usnctech
Copy link
Contributor

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
Copy link
Contributor Author

In digging around to find out if this is a bug or not, I was reminded of #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 keckler added the bug Something is wrong: Highest Priority label May 30, 2023
@keckler
Copy link
Member

keckler commented May 30, 2023

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
Copy link
Contributor Author

Bump on this nice to have feature

@john-science
Copy link
Member

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
Copy link
Member

john-science commented Feb 27, 2024

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":

pitch : float
Hex pitch (flat-to-flat) in cm

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
Copy link
Contributor

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.

@property
def pitch(self) -> float:
"""
Get the hex-pitch of a regular hexagonal array.
See Also
--------
armi.reactor.grids.HexGrid.fromPitch
"""
return self._unitSteps[1][1]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants