Optimizations and refactoring in mesh and cccc #174

ntouran · 2020-10-09T01:00:30Z

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.

* 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.

youngmit

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/nuclearFileMetadata.py

youngmit · 2020-10-09T15:53:28Z

armi/nuclearDataIO/cccc.py

+        # Need Metadata subclass for default keys
+        self.metadata = nuclearFileMetadata._Metadata()


Do you? There's always .get(key, None)...

Also, private class? Maybe just make it public?

good point. probably could use .get() in all the rw things. not sure why this wasn't done.

youngmit · 2020-10-09T15:56:42Z

armi/nuclearDataIO/cccc.py

+        self.metadata = nuclearFileMetadata._Metadata()
+
+
+class StreamWithDataContainer(Stream):


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.

it's nearly possible to merge these into one class but for some complexities in the big boys: isotxs, gamsor, pmatrx

youngmit · 2020-10-09T16:01:04Z

armi/nuclearDataIO/cccc.py

@@ -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.


from a CCCC.Stream. We are overloading a very meaningful term here with Stream, so should be careful.

youngmit · 2020-10-09T16:01:45Z

armi/nuclearDataIO/cccc.py

+    This is a relatively common pattern so some of the boilerplate
+    is handled here.


Might be worth discussing why not all CCCC.Streams use this pattern. Havent refactored yet?

youngmit · 2020-10-09T16:01:54Z

armi/nuclearDataIO/cccc.py

+
+class StreamWithDataContainer(Stream):
+    """
+    A stream that reads/writes to a specialized data container.


CCCC.Stream

youngmit · 2020-10-09T16:05:41Z

armi/reactor/reactors.py

@@ -618,7 +618,7 @@ def getNumAssembliesWithAllRingsFilledOut(self, nRings):
            return nRings * (nRings - 1) + (nRings + 1) // 2

    def getNumEnergyGroups(self):
-        """"
+        """ "


huh. good old new black version?

an extra quote is an extra quote. my guess is black is hesitant to actually remove it

armi/reactor/reactors.py

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.

youngmit

Looking better I think. Just a couple of things left and I'll be happy

armi/nuclearDataIO/cccc/__init__.py

youngmit · 2020-10-09T23:11:40Z

armi/nuclearDataIO/cccc/cccc.py


 import numpy

 from armi.localization import exceptions
 from armi import runLog

+from .. import nuclearFileMetadata
+
 IMPLICIT_INT = "IJKLMN"


hahaha, no way... does CCCC use the implicit type rules from fortran?

Not sure if it does actually use them, but it does seem that way based on all the naming + types so far

armi/nuclearDataIO/cccc/cccc.py

youngmit · 2020-10-09T23:24:01Z

armi/physics/neutronics/const.py

+    REAL = 0
+    ADJOINT = 1
+    REAL_AND_ADJOINT = 2


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;

or make this a flags, with auto()?

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.

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?

youngmit · 2020-10-09T23:27:18Z

armi/physics/neutronics/const.py

+        if opts.real and opts.adjoint:
+            return SolutionType.REAL_AND_ADJOINT
+        elif opts.real:
+            return SolutionType.REAL
+        else:
+            return SolutionType.ADJOINT


Flags would make this easier, too. if opts.real: typ |= REAL, if opts.adjoint: typ |= ADJOINT, no special case for both

youngmit · 2020-10-09T23:30:43Z

armi/nuclearDataIO/cccc/nhflux.py

+    def _getDataContainer() -> NHFLUX:
+        return NHFLUX()
+
+    def readWrite(self):


ugg and you cant make this private without having to re-do ISOTXS?

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.

youngmit · 2020-10-09T23:31:05Z

armi/nuclearDataIO/cccc/nhflux.py

+
+        Notes
+        -----
+        This method should be private but conflicts with ``_readWrite`` so we need a better name.


_readWriteImpl()?

impl, short for implementation; common idiom, in other languages anyways

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

youngmit · 2020-10-09T23:37:12Z

armi/nuclearDataIO/cccc/__init__.py

+release of ccccutils written in c++ from 2001. 
+
+"""
+from .cccc import *


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

youngmit · 2020-10-12T15:11:23Z

armi/physics/neutronics/const.py

@@ -55,28 +55,3 @@
 RESTARTFILES = "Restart files"
 INPUTOUTPUT = "Input/Output"
 FLUXFILES = "Flux files"
-
-
-class SolutionType(Enum):


youngmit · 2020-10-12T15:12:15Z

armi/__init__.py

@@ -400,6 +400,26 @@ def configure(app: Optional[apps.App] = None):
        runLog.raw(app.splashText)


+def applyAsyncioWindowsWorkaround():


Ought to be a separate PR, but I'll allow it

youngmit

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.

ntouran requested a review from youngmit October 9, 2020 01:00

Consolidate cccc boilerplate into new StreamWithDataContainer helper

6b9766d

youngmit suggested changes Oct 9, 2020

View reviewed changes

ntouran added 5 commits October 9, 2020 14:40

Convert NHFLUX to reader/writer and delete read-only CCCC code

b191090

Code review comments

3c10af4

Add NHFLUX writing unit test

61c6340

More comments in CCCC classes

91afee3

youngmit suggested changes Oct 9, 2020

View reviewed changes

ntouran added 3 commits October 9, 2020 21:56

Comment fixes

bba29bb

Add asyncio workaround for Windows and Python 3.8

7ec2cad

Re-export cccc modules in nuclearDataIO init

4efe0d2

youngmit reviewed Oct 12, 2020

View reviewed changes

youngmit approved these changes Oct 12, 2020

View reviewed changes

Change CCCC rwList to return ndarray

af578a7

This makes all non-scalar CCCC data ndarrays for consistency. Also updated new NhfluxStreams to deal with VARIANT options.

ntouran merged commit 74add6b into terrapower:master Oct 12, 2020

ntouran deleted the cccc2 branch October 12, 2020 20:32

youngmit mentioned this pull request Oct 15, 2020

Asyncio related notimplementederror in Windows with Python 3.8 coming from zmq #175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimizations and refactoring in mesh and cccc #174

Optimizations and refactoring in mesh and cccc #174

ntouran commented Oct 9, 2020

youngmit left a comment •

edited

Loading

youngmit Oct 9, 2020

youngmit Oct 9, 2020

ntouran Oct 9, 2020

youngmit Oct 9, 2020

ntouran Oct 9, 2020

youngmit Oct 9, 2020

youngmit Oct 9, 2020

youngmit Oct 9, 2020

youngmit Oct 9, 2020

ntouran Oct 9, 2020

youngmit Oct 12, 2020

youngmit left a comment

youngmit Oct 9, 2020

jakehader Oct 10, 2020

youngmit Oct 9, 2020

youngmit Oct 9, 2020

ntouran Oct 10, 2020

youngmit Oct 12, 2020

youngmit Oct 9, 2020

youngmit Oct 9, 2020

ntouran Oct 10, 2020

youngmit Oct 9, 2020

youngmit Oct 9, 2020

ntouran Oct 10, 2020

youngmit Oct 12, 2020

youngmit Oct 9, 2020

youngmit Oct 12, 2020

youngmit Oct 12, 2020

youngmit left a comment •

edited

Loading

		# Need Metadata subclass for default keys
		self.metadata = nuclearFileMetadata._Metadata()

		self.metadata = nuclearFileMetadata._Metadata()


		class StreamWithDataContainer(Stream):

		This is a relatively common pattern so some of the boilerplate
		is handled here.

		@@ -400,6 +400,26 @@ def configure(app: Optional[apps.App] = None):
		runLog.raw(app.splashText)


		def applyAsyncioWindowsWorkaround():

Optimizations and refactoring in mesh and cccc #174

Optimizations and refactoring in mesh and cccc #174

Conversation

ntouran commented Oct 9, 2020

youngmit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngmit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngmit left a comment • edited Loading

Choose a reason for hiding this comment

youngmit left a comment •

edited

Loading

youngmit left a comment •

edited

Loading