ARMI should have more abstract classes

[drewj-tp]

This might be my hottest take on ARMI from someone who has worked on the outside of ARMI using ARMI for a few years before starting to work on the inside of ARMI:

ARMI defaults to doing too much.

This is great but it can also be frustrating. Great because you have a lot of functionality on block and assembly and core objects for free. There are a lot of parameters that someone assumes you're going to want and sometimes you do and you don't have to re-define them.

Frustrating if you want to make an ARMI application purpose built for something that isn't

• sodium fast reactor (drives a lot of default parameters)
• hexagonal geometry (drives a lot of assumptions on grids and geometric accessors)
  • Cartesian geometry exists but still must align w/ a lot of hexagonal nomenclature like getRingPos for 2d location when something like getXY or getIJ would be more intuitive.
     - doing traditional two-stage neutronics (lattice physics multi-group xs generation followed by deterministic global flux solve)

I know this is where ARMI originated and that drove the design and a lot of the defaults. I have put some thoughts in a discussion on supporting more reactor types but I want to go a bit deeper.

What if ARMI provided pure abstract classes that only did the absolute bare minimum?

Case study

The block as a concept is a thing with child components and a height and lives in an assembly. Yet, the default armi.reactor.blocks.Block has functionality for

• getting/setting pin pitch,
• getting pin coordinates,
• smear density,
• generating a report of its area fractions,
• and more.

HexBlock inherits from Block and brings in a lot of its own assumptions. One of which is your pin pitch is based on a wire component which makes sense for SFRs but not for prismatic HTGRs that are hexagonal but don't have wire wraps.

These are useful but I would say they are not universally useful all the time. The API for a Block and its associated documentation is extensive.

What if ARMI shipped an AbstractBlock that provided very little functionality if at all but provided the interface, through abstractmethods for how the most basic block should be expected to behave?

• composite behavior like iterating over children, accessing the parent,
• getting the height of the block,
• (maybe) rotating
  • fuel shuffling is decently common across most reactors with assemblies.
  • Maybe or maybe not worth adding to the "definition" of a block
• necessary API for building from blueprints.
  • though that could and maybe should be separate.

Then, the AbstractHexBlock could provide a little more of the interface.

The current ARMI blocks could have a name more tied to their use case and engineering domain, like FastReactorHexBlock and SFRHexBlock.

Benefits

The composite tree structure is great. The plugin and hub-and-spoke model is powerful. This could be a way to further remove some of the modeling assumptions and allow applications to build their own plugins specific to their needs.

Applications don't have to spend time or see warnings because their blocks inherit from something that has different goals.

Users and plugins are more likely to use more of the API afforded to them. Methods that are not used still show up in docs and when people do tab completion in ipython looking for the method they can't quite remember.

Drawbacks

Lots.

This is a huge architectural change. It's going to be hard and take time and need to be well thought out and advertised and get lots of stakeholders on board. Internal and external.

Where do you put parameters? What parameters matter for a SFRHexBlock and should be defined there and not on a HTGRHexBlock or LWRCartesianBlock?

Determining what the bare functionality to be on an abstract block or component or assembly would be a community effort. If you're trying to make a (semi) general purpose Monte Carlo interface, like the openmc plugin, does this distinction make your life easier or harder?

I'm not convinced the reactor-specific block inheritance tree of AbstractBlock -> AbstractHexBlock -> FastReactorHexBlock -> SFRHexBlock is great.

Conclusion

I am a big fan of ARMI and its goal and design. I think this could unlock a lot of potential for people building with ARMI by reducing some friction and modeling assumptions. More on the modeling assumptions in #1894

[john-science]

I like the above. I would like to add some classic software clues that we don't have enough abstraction in our codebase. Let me focus on the data object model (at time of writing):

• class Core = 2311 lines
• class Block = 1677 lines
• class Assembly = 1221 lines

Perhaps there should be a simple Core, then we could add more detailed implemenations with helpful tools like HexCore and RectangularCore or PinTypeCore or FastHexCore and ThermalHexCore.

Such distinctions would make reading, understanding, and testing the code easier. But it would also allow for ARMI to better support a wider range of rectors.

[drewj-tp]

Fully on board that better abstraction could help ARMI support more types of reactors. I think the challenge will be determining that inheritance tree, what goes where, etc.

class Core = 2311 lines
class Block = 1677 lines
class Assembly = 1221 lines
class HexBlock = 848 lines

ooooooof

[john-science]

I have been staring at the bloat and overly-specific cruft in these data classes for ~3 years now. I feel comfortable we can define good layers of abstraction without any trouble.

[john-science]

Right now, I'm thinking:

• Block

  • HexBlock(Block)
  • CartesianBlock(Block)
  • ThetaRZBlock(Block)

• PinnedBlock(Block)

  • PinnedHexBlock(PinnedBlock, HexBlock)
  • PinnedCartesianBlock(PinnedBlock, CartesianBlock)

• FastPinnedHexBlock(PinnedHexBlock)

• Etc

Something like that. Right now our HexBlock/CartBlock have empty init methods (pass through to Block). If PinnedBlock was also a pass-through, this seems easy enough.

[john-science]

Or maybe the shapes don't have to subclass Block.

[drewj-tp]

A comment from @john-science in #1894 is worth pulling here

[If] some of your problems are that ARMI forces certain functionality through the default plugins in the default App, it is meant to be very easy to remove plugins from the default App

An AbstractArmiApp could be useful as well. I imagine most projects inherit from armi.app.App which brings in a lot of default plugins.

[drewj-tp]

Actually, as @drewj-tp and I [@john-science] have been talking lately about how to make ARMI more flexible, I think I want to go in the opposite direction this ticket suggests.
I think we want to make better use of our class heirarchies:

• Block

  • HexBlock(Block)
  • CartesianBlock(Block)
  • ThetaRZBlock(Block)

• PinnedBlock(Block)

  • PinnedHexBlock(PinnedBlock, HexBlock)
  • PinnedCartesianBlock(PinnedBlock, CartesianBlock)

• FastPinnedHexBlock(PinnedHexBlock)

• Etc

The idea being that right now ARMI's HexBlock is actually a "fast-reactor, pin-type, hex-shaped block". And ARMI forcing all this extra stuff on people without letting them just have a "PinnedHexBlock" to use for the thermal reactor is very limiting.

I'm not sure I like the multiple inheritance structure here. I think that if we want ARMI to be abstract we should make the minimum viable abstract type and then provide an open source plugin that extends the data model as an example for how one may create an application and build what is needed. Going down the path of pre making these inheritances will be messy quickly and implies that the framework knows or needs to know and handle a bunch of situations. I like the idea of the framework allowing for applications to extend the data model on initialization and just providing example documentation.

Thoughts?

From @jakehader in #303 (comment)

[jakehader]

I like the a lot better than creating further block abstractions. Thanks for putting the thought into it!

[john-science]

Drew, I like that idea.

It has a downside though: it will require massive PRs into every single downstream project that is based on ARMI.

One of the benefits of adding layers of abstraction is that you can reorganize code without removing any API endpoints, you just add to the API.

[john-science]

You originally called your ticket "ARMI should have more abstract classes", but here you seem to change your opinion and you think we should choose Composition over Inheritance?

We can chat offline about "composition vs inheritance".

Either way, I have a note about your above composition solution:

• I don't think we want to override __len__ and __iter__ directly.

My reasoning here is that people don't "just want pins", there are two completely separate things people want at any one time:

1. The fuel pins (or their number)
2. The total number of pins if their arrangement is geometrically complete (or their number)

Sure, if you want to calculate the total fuel, you just want the number of real pins. But if you are using the number of pins in a rotation calculation, you want that second thing. Right, you have several tickets and discussions open on this topic. I think we have to provide both (1) and (2), and not just assume len(pins) is sufficient for all users in the future.

[john-science]

Drew and I talked about this. We are thinking to combine composition and inheritance. Our end goals would be to make it easier for ARMI to support a wider range of reactors, in a more clear and easier to understand way.

Our current best-guess at the design:

• Core
  • AssemblyCore(Core)
  • HexAssemblyCore(AssemblyCore) <-- our current one
• Block
  • Block.pins <-- can be empty (there may be other physics ideas like this)
  • HexBlock(Block)
  • CartesianBlock(Block)
• Assembly
  • Assembly.blockType = HexBlock <-- keeps Assembly generic

@drewj-tp What do you think? (1) Is that enough layers of abstraction for your original desire in this Discussion? (2) What do you think of the specific breakdown itself?

[drewj-tp]

I'm really not sure on the inheritance structure. I like it a lot better than embedding pin-iness and spectrum into the type.

Hex/cartesian blocks are good enough. I think there's still value in a HexAssembly even if it's just an assembly that only contains HexBlocks.

[drewj-tp]

Bringing in another thought - #1030

Provide a BlockStack that captures a 1-D vertically sorted (bottom to top) collection of blocks. Then Assembly inherits from that to capture behavior an assembly in an LWR/SFR/HTGR might need (e.g., rotation).

A PBR or MSR would not have an assembly but could still benefit from a BlockStack that just describes a vertical slice of a region of the reactor

[drewj-tp]

Or, going further, CompositeStack (like proposed in the ticket) for generic 1-D stack of composites, then BlockStack, then Assembly

[drewj-tp]

Okay, some thoughts.

(SFR below is a stand-in for the pin-type hexagonal assembly fast spectrum sodium cooled reactor concepts that has informed a lot of ARMI's design decisions)

1. Determine if Composite can be made a proper abstract class. Is there any functionality that we expect a composite to implement on its own? It defines a rather large interface so maybe it's already fully featured enough?
2. Make Component a proper abstract class. Mark methods that currently raise NotImplementedError are marked as abstract methods.
   • Motivation: if you want to bring a new shape into ARMI, it is opaque (and sometimes frustrating) trying to get your new shape to play nice with the rest of ARMI when you don't know what you need to implement. An abstract class that errors trying to instantiate a semi-concrete class is an excellent use of the language.
3. Define abstract base classes for Block and Assembly
   • Motivation: if someone wants to bring their own application without a lot of the SFR-specific features and parameters, what should they provide?
4. Define abstract base class for core and how it would get constructed from blueprints
   • Motivation: Similar motivation for abstract blocks and assemblies: what would someone wanting to not model a SFR need on their Core
   • This might be even more open ended because someone could just attach something to a Reactor and then to an Operator and get the physics coupling just fine, so long as all their plugins know how to handle that specific thing attached to o.r.core

With these questions in place, we have the abstract (or semi-abstract) building blocks (pun intended) for the composite tree. Could these be a submodule, e.g., armi.reactor.bases? or armi.tree.bases? Signifying these have the base classes you should start subclassing from?

from armi.reactor.bases import BaseComponent, BaseBlock, BaseAssembly

class MyNewShape(Component):
...
class MyNewBlock(BaseBlock):
...
class MyNewAssembly(BaseAssembly):
...

Then, we could move a lot of the existing (SFR-influenced) classes in armi.reactor into maybe armi.reactor.sfr? And cartesian into armi.reactor.lwr? That may get too far outside of scope

[jakehader]

I actually don't think composition or inheritance is really an issue here, but rather we need to define what comes with and what comes adjacent to the framework more broadly for the reactor and physics packages. There are two main things we are looking to solve:

• Provide a suggested architecture for an application to build on.
• Provide some, batteries included so that the framework is not just an empty shell.

If we focus on the first for a minute, ARMI does have an architecture (that after reading the documentation and seeing examples applications) can make sense to a developer. The issue here though is that we haven't defined to what level we encourage individual applications to always, sometimes, or never need to configure. Defaults are fine as long as there are clear hooks in the code base (e.g., blueprints, settings, reactor package, etc.) where it's obvious for a developer to make a new subclass or to replace with what is needed.

As for the second, the amount of batteries included is a choice and we'd like to offer enough that it isn't burdensome to work with. Making decisions for all possible developers on Composite, Component, Block, and Assembly abstractions seems like we are going to far. Similarly, we only offer an Interface and an Operator with some examples subclasses. Arguably these should also be configurable by an application if said application doesn't like the out of the box assumptions.

What I'm specifically trying to express is while I think we have a lot of batteries included, maybe the right thing to do in this instance is to think about building this in the application and making the framework a bit more versatile towards generically serializing and deserializing the data structures that an application can build on into the database. The discussion around pins might be generic enough that it warrants extended the batteries included feature for the framework, but it might also be specific enough in its implementation and need that this is something where we figure out how to build and use it in the application.

If we think about the requirements for composition first and the feature set for which batteries are included next, and where those batteries live third (e.g. separate repositories in their own plugins, within the namespaces we already have, or some other architecture) it can help us think about the framework API a bit more deliberately and then applications can then chose their own path for coming back to the framework or the ecosystem to suggest a plugin or "batteries included" feature for the framework without constant framework updates based on unique situations.

It's my opinion that a lot of the batteries included features should just be in a plugin we offer since those are our opinions when someone is getting started. We want folks to be able to get off the ground as soon as possible without too much of a build your own adventure, but for users that want to fully customize their application we should also have the ability to stand aside and not limit their needs.

Intentionally building more shape or reactor specific implementations only pushes the framework into more of an opinionated, batteries included mode of thinking where I personally think based on experience with ARMI that we typically don't want that for internal TerraPower projects and since it just adds more maintenance burden to the system overall. Also, we technically should only be as good as our application requirements need us to be, so if we don't have a strong requirement or basic need to keep building a the out of the box Block or Assembly classes out and making an opinionated implementation for all starting users / developers let's make it so an application can easily subclass or override what is there and call out which are truly the API-level methods / functions that must be implemented if you choose to do this and put everything else somewhere else.

To me, this would actually help the framework stabilize it's API, it's testing, and it's maintenance longer term versus seeing a functionality that is sort of close to what in the framework and opening up many issues and pull request to change it here first and flow down to the application.

TLDR;

It's my opinion that new feature sets should start at the application level and then go to the framework over time only after it's carefully vetted by "the community" versus the other way around.

I could be pretty far off from everyone else, but that's how I'm interpreting this @drewj-tp and also the recent changes in some PRs.