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

added modern RZ theta unittest for ARMI #1009

Merged
merged 7 commits into from Dec 15, 2022
Merged

added modern RZ theta unittest for ARMI #1009

merged 7 commits into from Dec 15, 2022

Conversation

sombrereau
Copy link
Contributor

@sombrereau sombrereau commented Dec 5, 2022

Description

The ability to generate RZ theta ARMI models systematically from other interfaces. Is not functional. Therefore, a better unittest needs to be developed to lock in the capability to initialize RZ theta ARMI models defined with radial segments and volume fractions.


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 bug fixes or new features.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@john-science
Copy link
Member

I am pretty uncertain about this.

I have been told, several times, that we only support RZ Theta blueprints in the old Geom XML file format, and not in the YAML format. That is actually the only reason we haven't fully dropped support for the old geom files.

@john-science john-science added the feature request Smaller user request label Dec 6, 2022
@sombrereau
Copy link
Contributor Author

sombrereau commented Dec 6, 2022

I couldn't figure out how to go through the initialization with the geom file format, this passes with tests to check that perform operations that need to have the right block geometry in order to pass the unit tests. I looked back and my team has been previously using yaml geomFiles 9 months ago, but when I looked at this a few weeks ago I could no longer navigate through the reactor initialization.

@jakehader
Copy link
Member

@sombrereau - Godiva is a sphere model. Does it make sense here?

@sombrereau
Copy link
Contributor Author

@jakehader -- This input is demonstrating the capability of ARMI to model 3D features across a RZ theta geometry.

the central block (theta:0, radius:0, axial:2) has a volume fraction of HEU of 1 and in the upper corner (theta:0, radius:3, axial:4) the volume fraction is only 4.6% such that you define a pixelated "sphere".

I would be happy to entertain other RZ theta geometry benchmarks defined with this kind of strategy though. I just happened to have a Godiva model lying around.

@jakehader
Copy link
Member

Got it! I think it would be cool if the RZTheta grids could be updated to also just support 1D models, like sphere or slab but I see what you're getting at. Want to see if this model runs as-is with PARTISN and check k-eff?

@jakehader
Copy link
Member

I couldn't figure out how to go through the initialization with the geom file format, this passes with tests to check that perform operations that need to have the right block geometry in order to pass the unit tests. I looked back and my team has been previously using yaml geomFiles 9 months ago, but when I looked at this a few weeks ago I could no longer navigate through the reactor initialization.

I think that the system for R, RZ, and RZT needs some love and that would a good PR as a predecessor to this in my opinion. See @john-science's comment on #450 (comment)

@john-science
Copy link
Member

I definitely agree with @jakehader that we need better support for R, Z, and RZTheta geometries in our YAML formats.

@john-science
Copy link
Member

@ntouran What do you think about this? We could add in a couple of other semi-real-world examples. (We mentioned the Demon Core yesterday.) And we could just store them all in the same folder.

@john-science
Copy link
Member

@sombrereau Seeing all these "void" really highlights that ARMI doesn't have an "air" material. Nor even a "pure N gas" material, which would at least be 78% correct. Thanks!

@sombrereau
Copy link
Contributor Author

I added a new issue. I might add something simple ... at sometime.

@john-science john-science merged commit 0c6d0ae into main Dec 15, 2022
@john-science john-science deleted the rztheta_unittest branch December 15, 2022 22:00
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