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

`o.snapshotRequest` is confusing and misleading #1472

Open albeanth opened 10 months ago

albeanth commented 10 months ago

Making a quick note to try and address this.

This method is strange and feels out of place. Maybe it has to live on the Operator, but it's odd....

https://github.com/terrapower/armi/blob/78477048c1f9d331bc531351bd450fe82d786e5e/armi/operators/operator.py#L1063-L1076

The docstring calls out that the concept of "snapshots" has evolved since this method was written - and as is, is quite confusing relative to the snapshot operator. I would propose that this method (if possible) get moved over to the snapshotInterface -- and if possible renaming the snapshotInterface to something more accurate with its current intended usage. Something like dumpInputInterface or something like that....

albeanth commented 10 months ago

@keckler

john-science commented 5 months ago

Sure, I like this.

The method Operator.snapshotRequest() only needs two class attributes from the Operator: the Reactor and the CaseSettings. These will both be easily available in the SnapshotInterface, so there will be no problems moving the method over.

I only see snapshotRequest() used once outside of ARMI, so this should be an easy change to make.

albeanth commented 5 months ago

@john-science any thoughts on renaming the snapshotInterface too?

and if possible renaming the snapshotInterface to something more accurate with its current intended usage. Something like dumpInputInterface or something like that....

john-science commented 3 weeks ago