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

Sort Assembly by ring position #1305

Closed
wants to merge 6 commits into from
Closed

Sort Assembly by ring position #1305

wants to merge 6 commits into from

Conversation

onufer
Copy link
Member

@onufer onufer commented Jun 13, 2023

Description

The sorted order of assemblies in a reactor doesn't make a whole lot of intuitive sense to the user. the current ordering is dervied from the fact that its faster for python code to determine neighbors by completeIndicies. But complete indices are more of an implementation detail that is not exposed to users. When you print an assembly you get ring/position not indices.

Here is a screenshot of old sort on left and new sort on right (for the test reactor). you can see how the numbers are increasing and make more sense, and the assembly types most folks care about (fuel and control) are at the front. It also makes spatial sense since its innermost to outermost.:

image


Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
  • The release notes (location doc/release/0.X.rst) are up-to-date with any important changes.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@john-science john-science added the feature request Smaller user request label Jun 13, 2023
@john-science
Copy link
Member

john-science commented Jun 13, 2023

My first thought on this change is that not all assemblies are in reactors that have rings. For instance, we can build a fun little example of Chicago Pile 1 with ARMI, and that has square assemblies that aren't in rings.

Perhaps you meant to make these changes on HexAssembly? It would seem a much stronger assumption that reactors with HEX-shaped assemblies have rings, right?

@john-science john-science marked this pull request as draft June 13, 2023 16:37
@onufer
Copy link
Member Author

onufer commented Jun 13, 2023

sorry this should be marked draft. i will work on the description. i hadn't assigned a reviewer yet so didnt mean to spin you up

i do believe all geometreis support getRingPos, so no problem with CP1 or LWRs if we can get them in armi @john-science. it also makes sense for them too, since your first assembly will be fuel rather than some graphite on the outside or periphery of lwr.

image

@onufer onufer marked this pull request as ready for review June 13, 2023 19:08
@onufer
Copy link
Member Author

onufer commented Jun 17, 2023

this causes many internal test differences. i do think this is the long term best way to go, but abandoning for now in favor of #1320

@onufer onufer closed this Jun 17, 2023
@opotowsky opotowsky deleted the changeAssemOrder branch October 10, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants