-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimizations and refactoring in mesh and cccc #174
Conversation
* Add a implicit-typed list reader to CCCC utils since we have 3 implementations, and growing * Make CCCC file names into module-level constants to reduce strings * Reduce pointless mesh computations during reactor init * Promote coverage to main requirements file since it is required if you activate the framework "coverage" setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cuts down on some boiler plate, but I'm worried that CCCC is getting out of hand. It isn't clear where implementation details stop and client-facing APIs begin. The implementation is perfectly fine, but it is rather baroque, and exactly the kind of thing that "hide implementation details" is all about. We should probably push a lot of this stuff down into a sub-package of nuclearDataIO, and expose the public bits in its __init__.py
Seems to me like the only thing a user should see is a bunch of data container classes (one per file type), which have class methods for reading/writing from file/stream, plus whatever bespoke stuff each file should have based on its intended use.
armi/nuclearDataIO/cccc.py
Outdated
# Need Metadata subclass for default keys | ||
self.metadata = nuclearFileMetadata._Metadata() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you? There's always .get(key, None)
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, private class? Maybe just make it public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. probably could use .get() in all the rw
things. not sure why this wasn't done.
self.metadata = nuclearFileMetadata._Metadata() | ||
|
||
|
||
class StreamWithDataContainer(Stream): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be private. IMO the only thing that should be public out of CCCC is one data container class per file type with public fromFile/Stream and toFile/Stream class methods. And maybe the metadata stuff? Everything else is implementation trickery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's nearly possible to merge these into one class but for some complexities in the big boys: isotxs, gamsor, pmatrx
armi/nuclearDataIO/cccc.py
Outdated
@@ -689,6 +706,57 @@ def _write(cls, lib, fileName, fileMode): | |||
raise NotImplementedError() | |||
|
|||
|
|||
class DataContainer: | |||
""" | |||
Data representation that can be read/written to/from with a Stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from a CCCC.Stream
. We are overloading a very meaningful term here with Stream, so should be careful.
This is a relatively common pattern so some of the boilerplate | ||
is handled here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth discussing why not all CCCC.Streams use this pattern. Havent refactored yet?
armi/nuclearDataIO/cccc.py
Outdated
|
||
class StreamWithDataContainer(Stream): | ||
""" | ||
A stream that reads/writes to a specialized data container. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CCCC.Stream
armi/reactor/reactors.py
Outdated
@@ -618,7 +618,7 @@ def getNumAssembliesWithAllRingsFilledOut(self, nRings): | |||
return nRings * (nRings - 1) + (nRings + 1) // 2 | |||
|
|||
def getNumEnergyGroups(self): | |||
"""" | |||
""" " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh. good old new black version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an extra quote is an extra quote. my guess is black is hesitant to actually remove it
This is another step towards a better cccc submodule. * Put all cccc modules in new subpackage for clarity about what's cccc * Added more docs and examples This did not go so far as to add a consistent public interface on all cccc DataContainers so users don't have to use the streams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better I think. Just a couple of things left and I'll be happy
|
||
import numpy | ||
|
||
from armi.localization import exceptions | ||
from armi import runLog | ||
|
||
from .. import nuclearFileMetadata | ||
|
||
IMPLICIT_INT = "IJKLMN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hahaha, no way... does CCCC use the implicit type rules from fortran?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it does actually use them, but it does seem that way based on all the naming + types so far
armi/physics/neutronics/const.py
Outdated
REAL = 0 | ||
ADJOINT = 1 | ||
REAL_AND_ADJOINT = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make these 1, 2 and 3? It would let us transition a bitfield and literally treat REAL_AND_ADJOINT as REAL & ADJOINT, as in Flags. Imagine a code that doesnt know how to do both at the same time, it wouldnt need a special case to do both, it could do if REAL in solType, do real solve; if ADJOINT in solType do adjoint solve;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or make this a flags, with auto()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a few code-specifics ones of these in an internal library and they correspond to specific values. Maybe I could move this back out of the framework because you're right about it not being great for flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pick one. If we do it on the framework side, we need to do it right. Plugins can do what they want. Flags/bitfield is 100% the way to go. Maybe we can address this in a separate PR?
armi/physics/neutronics/const.py
Outdated
if opts.real and opts.adjoint: | ||
return SolutionType.REAL_AND_ADJOINT | ||
elif opts.real: | ||
return SolutionType.REAL | ||
else: | ||
return SolutionType.ADJOINT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flags would make this easier, too. if opts.real: typ |= REAL
, if opts.adjoint: typ |= ADJOINT
, no special case for both
def _getDataContainer() -> NHFLUX: | ||
return NHFLUX() | ||
|
||
def readWrite(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugg and you cant make this private without having to re-do ISOTXS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this readWrite
thing being public is one current bane of the cccc subpackags. It needs a new name and it needs to be private. It can't be _readWrite
because we already have one. I was considering _readWriteMain
, etc. for a while.
Redoing isotxs won't be too hard. It's pretty close.
|
||
Notes | ||
----- | ||
This method should be private but conflicts with ``_readWrite`` so we need a better name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_readWriteImpl()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl, short for implementation; common idiom, in other languages anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes I wrote that. Uh sure that'd be fine. I want to change all of them at once to do that so that m ight be in phase 5 cccc cleanup pr if that's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
release of ccccutils written in c++ from 2001. | ||
|
||
""" | ||
from .cccc import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can tighten this up later, but this is the perfect place to serve as an aperture to control what a client sees within the package. Lots of the submodules themselves could be private (e.g. _nhflux.py), with the necessary (public) NHFLUX classes being imported/reexported here. Future work
armi/physics/neutronics/const.py
Outdated
@@ -55,28 +55,3 @@ | |||
RESTARTFILES = "Restart files" | |||
INPUTOUTPUT = "Input/Output" | |||
FLUXFILES = "Flux files" | |||
|
|||
|
|||
class SolutionType(Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -400,6 +400,26 @@ def configure(app: Optional[apps.App] = None): | |||
runLog.raw(app.splashText) | |||
|
|||
|
|||
def applyAsyncioWindowsWorkaround(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ought to be a separate PR, but I'll allow it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. So next steps:
- make stream classes private,
- put public interface on data containers
- rename readWrite and make it private
of those i think the readWrite thing is the highest priority. that the streams are public is a bit odd, but that its public interface is mostly a fused readWrite function is challenging from a documentation/understanding perspective.
This makes all non-scalar CCCC data ndarrays for consistency. Also updated new NhfluxStreams to deal with VARIANT options.
Add a implicit-typed list reader to CCCC utils since we have 3
implementations, and growing
Make CCCC file names into module-level constants to reduce strings
Reduce pointless mesh computations during reactor init
Promote coverage to main requirements file since it is required if you
activate the framework "coverage" setting.