HexBlock.autoCreateSpatialGrids raises lots of spurious exceptions

[drewj-tp]

It's not uncommon for me to see messages like

[warn] Could not create a spatialGrid for block BLOCK, multiplicities are not 1 or N they are {1}

which, what? I read this as "multiplicities are not 1 they are 1" which enforces a sense of distrust in warning messages.

Tracking this down, we try/except Block.autoCreateSpatialGrids when creating a block in blueprints

armi/armi/reactor/blueprints/blockBlueprint.py

Lines 243 to 247 in f1f3dbe

	if b.spatialGrid is None and cs[globalSettings.CONF_BLOCK_AUTO_GRID]:
	try:
	b.autoCreateSpatialGrids()
	except (ValueError, NotImplementedError) as e:
	runLog.warning(str(e), single=True)

and the HexBlock implementation throws this error

armi/armi/reactor/blocks.py

Lines 2353 to 2358 in f1f3dbe

	if len(mults) != 2 or 1 not in mults:
	raise ValueError(
	"Could not create a spatialGrid for block {}, multiplicities are not 1 or N they are {}".format(
	self.p.type, mults
	)
	)

The error appears to be triggered because we don't have two multiplicities. Which I guess would be what you'd expect an assembly with pins to have: single multiplicity for a duct or other bounding component, and then a multiplicity > 1 for pin-like components.

Remedy

If there is only a single multiplicity, and it's close to one, do not attempt to make a spatial grid on the block. This feels acceptable. There's nothing here to make a spatial grid so we won't. It's not that we cannot make a spatial grid (e.g., invalid number of pins), just that it doesn't make sense to create on here. Maybe the autocreate method raises an exception that is silently discarded in the block blueprint?

[keckler]

+1 on this issue. I see this warning ~10 bazillion times whenever I run, and the warning message is not very useful.

I would also say that the warning is not entirely justified, considering that nobody guaranteed the user that a spatial grid would be created on their block.

I guess this message feels more like a "extra" or "debug" kind of thing, to me.

[drewj-tp]

I would also say that the warning is not entirely justified, considering that nobody guaranteed the user that a spatial grid would be created on their block.

Totally get the confusion, especially for things that don't make sense to have grids.

Surprise surprise, there's a setting, defaulting to True, that controls this

armi/armi/settings/fwSettings/globalSettings.py

Lines 832 to 839 in f1f3dbe

	setting.Setting(
	CONF_BLOCK_AUTO_GRID,
	default=True,
	label="Auto-generate Block grids",
	description="Should block blueprints attempt to auto-generate a spatial "
	"grid upon construction? This feature makes heavy use of multi-index "
	"locations, which are not yet universally supported.",
	),

[john-science]

Well, I like an easy one.

I think, perhaps, we should:

Make the default for autoGenerateBlockGrids / CONF_BLOCK_AUTO_GRID False.

A quick review tells me that no one anywhere in the world uses this setting. So, if it's only job is to turn on this log statement, then fine the log statement can stay info level, IF the setting is off by default.

Alternatively:

1. We could remove the setting, it's nigh useless.
2. We could just move this log statement to debug.

Thoughts?

[keckler]

Huh, didn't know about that setting! In light of that, I guess the warning message makes a little bit more sense then.

I think that the assigning of auto-grids to the blocks is useful, ,so even if the default was changed for the setting, I would just turn it back on. And in light of this behavior being controlled by a setting, it feels a bit strange to bury the warning behind a debug print.

Hmmm...

Maybe the answer here is to have a runLog.warning that says something like "Warning: 500 blocks could not auto create a grid", and then separate runLog.extra messages that are for the specific blocks and the reasoning that the grid could not be created.

No matter, the message statement that Drew originally posted is cryptic and should be made more instructive for the user.

[drewj-tp]

A quick review tells me that no one anywhere in the world uses this setting

I think the answer is sneakier. No one turns the setting off or on explicitly, but I'm 100% certain people rely on the functionality this setting controls. We use it to get a spatial grid so we can have Block.getPinCoordinates be meaningful.

The alternative to not autosetting grids is to have a grids definition for every block in your reactor. Which would be extremely cumbersome. In simple cases, where you have the same pin-like thing at every lattice site in your reactor, and you have a mult that would fill an entire hexagonal grid, the auto creation of spatial grids is a huge time saver.

Maybe the answer here is to have a runLog.warning that says something like "Warning: 500 blocks could not auto create a grid"

That's a step in the right direction I think. Slap a single=True and `label="Cannot auto create spatial grid".

I would wager that 500 of those 500 blocks do not need a spatial grid. Because they are just blocks with single mult components. I would say there should be no warning and no grid created if mult == {1, }. That's not a thing where it makes sense to have a grid, and it's easy to detect

[john-science]

That's a step in the right direction I think. Slap a single=True and `label="Cannot auto create spatial grid".

Yeah, that's a good idea.

[keckler]

Note that the setting mentioned above (CONF_BLOCK_AUTO_GRID, otherwise known as autoGenerateBlockGrids) was removed here: https://github.com/terrapower/armi/pull/1947/files#r1797329551

[keckler]

One way to potentially cut down on the messages would be to move this line up to the top of the autoCreateSpatialGrids() method:

armi/armi/reactor/blocks.py

Line 2322 in 1920ff0

if self.spatialGrid is None:

That method is pretty inefficient, in the sense that it will do almost all of the work before even checking if the block needs a spatial grid. In cases where the block already has one, we can skip all the work entirely.

[john-science]

Honestly, I have not fully tested Chris' idea to move the if statement up. The original version of the code I was working from had this if statement so far down the method for good reason.

But it certainly looks like this check could be done at the top of the method now, with no problems.