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

Remove references to geom files in the code #450

Open
youngmit opened this issue Oct 20, 2021 · 12 comments
Open

Remove references to geom files in the code #450

youngmit opened this issue Oct 20, 2021 · 12 comments
Assignees
Labels
cleanup Code/comment cleanup: Low Priority enhancement New feature or request

Comments

@youngmit
Copy link
Contributor

Back in the day, we used to specify core layout in what was called a facemap or geom file. While we retain the code that supports this input mechanism (see https://github.com/terrapower/armi/blob/master/armi/reactor/systemLayoutInput.py), it is mostly there to enable the migration performed here:

def migrate(bp: Blueprints, cs):

Instead we recommend using grid blueprints instead. However, when these were completely separate input files, they became part of some major ARMI function signatures where input stuff gets passed around, usually as geom. We should try to remove as many of these as we can.

Actually searching around, I'm not seeing as many as I remember (go us!), but a glaring example is in the Case class:

def __init__(self, cs, caseSuite=None, bp=None, geom=None):

@john-science john-science self-assigned this Oct 20, 2021
@john-science john-science added the cleanup Code/comment cleanup: Low Priority label Oct 20, 2021
@john-science
Copy link
Member

What about this Database method:

def writeInputsToDB(self, cs, csString=None, geomString=None, bpString=None):

and

def writeInputsToDB(self, cs, csString=None, geomString=None, bpString=None):

Are those geomString arguments still necessary?

@john-science john-science removed their assignment Nov 30, 2021
@john-science john-science self-assigned this Mar 16, 2022
@john-science
Copy link
Member

john-science commented Mar 16, 2022

@drewejohnson @keckler @jakehader Would any of you notice or care if ARMI finally dropped all support for the old Geometry XML files?

First off, I'm not sure we have full, functioning support for the old XMLs any more. I assume I can nuke all references and support for the old Geometry XML files and it won't bother anyone.

Tell me if I'm wrong.

@keckler
Copy link
Member

keckler commented Mar 16, 2022

In fact, I would love it if XML support was dropped.

@john-science
Copy link
Member

john-science commented Apr 5, 2022

The following files will need to be re-written in YAML and moved into a blueprints file:

  • armi/tests/geom1Assem.xml
  • armi/tests/geom.xml
  • armi/tests/ThRZGeom.xml
  • armi/tests/trz_geom.xml
  • armi/tests/zpprTestGeom.xml

Though, maybe some/all of them will be straight deletable.

@keckler
Copy link
Member

keckler commented Sep 26, 2022

So, can you remind me the status of this?

I know we removed support for XML case settings in #612 .

But did we ever remove support for the XML geom file? I know that Jake said he would be fine with it, but did it ever actually happen? I can't find any record of that being finally deprecated.

@john-science
Copy link
Member

john-science commented Sep 27, 2022

I would love to remove geom files entirely.

There are still a couple of features blueprint files don't support that need translating over. I can't find my notes at the moment, but if memory serves, ARMI only supports ThetaRZ geometries through geomFiles, and not blueprint YAML files.

So, the last couple geomFile-only features need to be supported by blueprint YAMLs, then we need to help everyone downstream translate their inputs, THEN we can finally remove geomFiles.

@john-science
Copy link
Member

Okay, I can only find the one feature left unsupported by blueprint files: ThetaRZ reactors. So, these two files:

  • armi/tests/trz_geom.xml
  • armi/tests/ThRZGeom.xml

So, we need a blueprint/YAML format to replace the old XML for ThetaRZ:

<reactor geom="ThetaRZ" symmetry="full core">
<assembly theta1="0" theta2="360.0" rad1="0" rad2="2.0" name="IC" azimuthalMesh="1" radialMesh="1"/>
<assembly theta1="0" theta2="180" rad1="2.0" rad2="3.0" name="MC" azimuthalMesh="1" radialMesh="1"/>
<assembly theta1="180" theta2="360" rad1="2.0" rad2="3.0" name="MC" azimuthalMesh="1" radialMesh="1"/>
<assembly theta1="0" theta2="360" rad1="3.0" rad2="4.0" name="OC" azimuthalMesh="1" radialMesh="1"/>
</reactor>

@john-science
Copy link
Member

john-science commented Oct 7, 2022

I believe the YAML version of the ThetaRZ geom/xml file should look something like:

assemblies:
heights:
- 3.5
- 3.5
- 3.5
- 3.5
- 3.5
axial mesh points:
- 5
- 5
- 5
- 5
- 5
assembly1_1:
specifier: assembly1_1
blocks:
- name: block1_1_1
Godiva:
shape: RadialSegment
material: UZr
Tinput: 26.85
Thot: 26.85
outer_theta: 0.7853981633974483
height: 3.5
inner_theta: 0.0
outer_radius: 3.001
inner_radius: 0.0
mult: 0.9226919412612915
compliment:
shape: RadialSegment
material: Void
Tinput: 0.0
Thot: 0.0
outer_theta: 0.7853981633974483
height: 3.5
inner_theta: 0.0
outer_radius: 3.001
inner_radius: 0.0
mult: 0.0773080587387085

@john-science
Copy link
Member

According to @ntouran , the place in the code I need to improve to remove these geom files is in gridBlueprints.py, where we currently read the geom XML file versions of the RZ Theta blueprints.

That will have to be improved to fully support all the little bits and pieces we use for RZ Theta XML blueprints.

@john-science
Copy link
Member

Okay, we definitely support basic RZTheta geometries in YAML format.

Nick believes there are some extra pieces of information in the RZTheta geom files that isn't supported in RZTheta YAML files. So I need to find the specifics of what we don't support, and build support for it.

@john-science
Copy link
Member

john-science commented Aug 22, 2023

This is still quite important. Though the features that XML geomFiles support for RZ-Theta geometries are still unknown to me.

I believe the problem is something like: RZTheta "geomFiles" have some undocumented feature they support that isn't supported in our YAML blueprints.

But the "undocumented feature" is the problem.

Per our previous discussion: @opotowsky @albeanth

@john-science john-science added the enhancement New feature or request label Aug 22, 2023
@albeanth
Copy link
Member

albeanth commented Sep 6, 2023

Is there anyway we can give a deadline for folks to chime in on this? Then after the deadline we yank out the geom files and be done with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants