Is there any reason we need the nCycles case setting?

[keckler]

nCycles seems redundant to me. A quick search through the code base makes me think that we could easily just infer the value from the various cycle history case settings.

But before I advocate getting rid of it entirely, does anybody see a reason why it needs to exist as its own setting? I think it should still be kept internally to avoid having to do something like len(cycleLengths) anytime we need the number of cycles, but it could just be inferred at the start of a run and stored in the case settings data structure where it currently is.

[john-science]

I am curious what others think of this. Off the top of my head, I don't see any good reason not to remove it.

• The easiest solution would be to immediately calculate it at the start of a run. This allows us to change the least amount of code in ARMI.
• But if it really is just len(cycleLenths), and we can say 100% of the time people will have cycleLenghts, I am actually okay with a large PR where we just remove it.

Do we even validate that nCycles and cycleLengths agree right now? If not, this is another reason why reducing the redundancy of our inputs is a good idea.

[john-science]

Actually, after talking to people, it seems like nCycles is convenient for most people. It's nice to be able to see at a glance how many cycles you have, and if you have a cycleLengths in the dozens or hundreds of items long, counting them by hand is tedious. And a lot of people are currently defining their run by using configurations like nCycles: 12 and cycleLength: 2000.0. In this use case, having nCycle is consider quite convenient.

[keckler]

That is fine. I see why someone might want to enter input in that manner.

I guess what bothers me is that we force the input file to have redundant information by checking that availabilityFactors, powerFractions, and cycleLengths all have lengths that match nCycles through the use of _factorsAreValid:

armi/armi/operators/settingsValidation.py

Lines 465 to 504 in 035711a

	def _factorsAreValid(factors, maxVal=1.0):
	try:
	expandedList = expandRepeatedFloats(factors)
	except (ValueError, IndexError):
	return False
	return (
	all(0.0 <= val <= maxVal for val in expandedList)
	and len(expandedList) == self.cs["nCycles"]
	)
	self.addQuery(
	lambda: self.cs["availabilityFactors"]
	and not _factorsAreValid(self.cs["availabilityFactors"]),
	"`availabilityFactors` was not set to a list compatible with the number of cycles. "
	"Please update input or use constant duration.",
	"Use constant availability factor specified in `availabilityFactor` setting?",
	lambda: self._assignCS("availabilityFactors", []),
	)
	self.addQuery(
	lambda: self.cs["powerFractions"]
	and not _factorsAreValid(self.cs["powerFractions"]),
	"`powerFractions` was not set to a compatible list. "
	"Please update input or use full power at all cycles.",
	"Use full power for all cycles?",
	lambda: self._assignCS("powerFractions", []),
	)
	self.addQuery(
	lambda: (
	self.cs["cycleLengths"]
	and not _factorsAreValid(self.cs["cycleLengths"], maxVal=1e10)
	),
	"The number of cycles defined in `cycleLengths` is not equal to the number of cycles in "
	"the run `nCycles`."
	"Please ensure that there is exactly one duration for each cycle in the run or use "
	"{} days for all cycles.".format(self.cs["cycleLength"]),
	"Use {} days for all cycles?".format(self.cs["cycleLength"]),
	lambda: self._assignCS("cycleLengths", []),
	)

If we could optionally resolve nCycles at run time, for cases where it can be inferred, and change the _factorsAreValid check to instead just check that all three vectors are consistent lengths, that would be nicer, in my opinion.

[john-science]

Thoughts:

1. I'd be happy to change this method so that it allows for the check that all three have the same length in the case where nCycles isn't defined. Thus making nCycles potentially optional.
2. But we'd still need to do some work to ensure that nCycles is calculated on-the-fly for backwards compatibility reasons. This is, obviously, not hard to do. It would just be a matter of finding WHEN in the workflow to calculate it, to preserve our current functionality.

[ntouran]

Yeah I think things in the code should just check the internal cycleLengths data structure in all cases. It is still nice to have the user input option of using nCycles if you're inputting e.g. 300 cycles of the same cycle length for equilibrium analysis so I think we should retain the setting for that reason only. But all look-ups of the setting should be migrated to just check length of cycleLengths. Could make a property on the operator for convenience if we wanted.