Use clearer input syntax for hexagonal lattice pitch

mgjarrett commented 4 months ago

What is the change?

Added an option for a user to specify a hexagonal pitch directly:

latticePitch: {hex: 1.5}

Why is the change being made?

Previously, a user had to specify the triangular pitch using the x attribute of latticePitch, which is confusing because the pitch is not necessarily equal to the x distance between two neighbors in a hex grid.

latticePitch: {x: 1.5}

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

~Fix 1284~ (fix for this issue pulled into a different PR, #1649)
Close #252

Checklist

[x] This PR has only one purpose or idea.
[x] Tests have been added/updated to verify any new/changed code.

[x] The code style follows good practices.
[x] The commit message(s) follow good practices.

[ ] The release notes have been updated if necessary.
[ ] The documentation is still up-to-date in the doc folder.
[x] The dependencies are still up-to-date in pyproject.toml.

john-science commented 4 months ago

I'm just thinking aloud here, but what something "flat-to-flat" be better for hex grids?

Isn't the goal to call out that we are going to be explicitly picking one over the other?

mgjarrett commented 4 months ago

I'm just thinking aloud here, but what something "flat-to-flat" be better for hex grids?

Isn't the goal to call out that we are going to be explicitly picking one over the other?

I'm not sure I understand what you are proposing. Do you mean specifying the pitch/flat-to-flat distance as flat-to-flat in the blueprints instead of latticePitch? I think latticePitch is still a fine descriptor of that parameter for a hexagonal lattice. And this allows us to not break any backwards compatibility with existing blueprints files, which would be an enormous pain.

mgjarrett commented 3 months ago

The bug fix was pulled out of this PR and into #1649

The remaining part of this PR is an optional/nice-to-have feature. It's just allowing users to define the hex pitch in blueprints in a slightly different way than before. No rush to push this through now, really.

john-science commented 3 months ago

The unit tests broke after my PR merged. Sorry!

mgjarrett commented 3 months ago

The unit tests broke after my PR merged. Sorry!

No worries! I just have to pull out what was moved over to #1649 and I think it will work.

terrapower / armi