terrapower / armi

An open-source nuclear reactor analysis automation framework that helps design teams increase efficiency and quality
https://terrapower.github.io/armi/
Apache License 2.0
222 stars 87 forks source link

Plugin hook for initial axial expansion #1843

Open drewj-tp opened 2 weeks ago

drewj-tp commented 2 weeks ago

There isn't an easy way to override the initial axial expansion performed on assemblies at construction. Providing a mechanism like this would be useful.

I propose a plugin hook like

@staticmethod
@HOOKSPEC
def initialAssemblyAxialExpansion(assems: list[Assembly], cs: Settings) -> bool:
    pass

where

We don't want to perform multiple expansions, so when iterating over the hooks, we would cease iteration after the first call returned true. An example of this would be if the inputs heights considered hot flag is true, the default implementation would not perform expansion at construction and return false.

The default implementation would call this section of code

https://github.com/terrapower/armi/blob/6fd89e1b095c6290195967674758df0c64434caa/armi/reactor/blueprints/__init__.py#L314-L332

and be marked with the trylast argument. This would allow other plugins to potentially perform their expansion earlier, and then return True to signify no other expansion should take place.

john-science commented 2 weeks ago

@albeanth @drewj-tp I am hesitant to do tickets like this.

My problem is that there is still not documentation in ARMI for the Axial Expansion tooling. And without that, I can not really vet the design. We have over a dozen "axial expansion"-related tickets, and I lack the high-level design to really put them all in perspective.

john-science commented 2 weeks ago

@drewj-tp Quick Design Question about your proposal

def initialAssemblyAxialExpansion(assems: list[Assembly], cs: Settings) -> bool:

By my count, only half the reactor projects that are currently using ARMI have assemblies. So, the above feels awkward. Imagine if there was a plugin hook called "setNumberOfHexAssemblies"; that would be really odd for teams that have square assemblies or no assemblies, right? It would seem nicer if we could more generically insert logic into the interface stack.

What about the existing plugin hooks?

albeanth commented 2 weeks ago

Just adding two cents here -- this issue here is related to this discussion https://github.com/terrapower/armi/discussions/1824

albeanth commented 2 weeks ago

My problem is that there is still not documentation in ARMI for the Axial Expansion tooling. And without that, I can not really vet the design. We have over a dozen "axial expansion"-related tickets, and I lack the high-level design to really put them all in perspective.

😭

someday I'll have time....

...sigh....

drewj-tp commented 2 weeks ago

By my count, only half the reactor projects that are currently using ARMI have assemblies. So, the above feels awkward. Imagine if there was a plugin hook called "setNumberOfHexAssemblies"; that would be really odd for teams that have square assemblies or no assemblies, right? It would seem nicer if we could more generically insert logic into the interface stack.

That's fair. I can take a look at moving the expansion to afterConstructionOfAssemblies. I don't know how that would ensure two plugins don't both expand assemblies though

keckler commented 2 weeks ago

This is a bit orthogonal, but I have long thought that axial expansion should be moved to its own plugin. That plugin could default ship with ARMI, sure. But it would make the process more clear, and it would solve your problem with making sure it happens only once.

I also would like for ALL thermal expansion to be in a plugin, but the radial stuff is so deeply embedded that I think that'd be nigh impossible.

drewj-tp commented 2 weeks ago

This is a bit orthogonal, but I have long thought that axial expansion should be moved to its own plugin. That plugin could default ship with ARMI, sure. But it would make the process more clear, and it would solve your problem with making sure it happens only once.

This might be a more practical approach. Then apps could choose to unregister the default axial expansion plugin and bring their own. Or leave it out entirely 🤔

john-science commented 2 weeks ago

Hmm... Chris has mentioned this before, but moving axial expansion to an ARMI plugin "in ARMI" is a new twist. That would allow for the code separation Tony wants, but would also make it easy for users to just "turn off" and ignore axial expansion, if their reactor doesn't have assemblies.

That an interesting twist, man.

albeanth commented 2 weeks ago

@john-science if you have the bandwidth to pull axial expansion out and into a plugin... that would be great! It is physics after all.

drewj-tp commented 1 week ago

@albeanth @john-science let's talk about this offline. I'm interested but I think there's a lot of potential gotchas