Various changes in Python API

[paulromano]

This pull request makes a few changes/additions:

• Universe.plot now has two extra arguments, colors (dictionary mapping cells/materials to RGB(A) tuples) and filename (for saving plot to file instead of display).
• Most __deepcopy__ methods have been removed since they don't do anything special. I'm holding off on making more radical changes like autogenerating new IDs for a deepcopy since we don't have consensus on it yet.
• A new openmc.model subpackage was added with a TRISO class that helps ease modeling TRISO fuel particles. There's also a create_triso_lattice function which is loosly based on @smharper's one-off script for placing TRISO particles in a lattice, duplicating them where they overlap lattice boundaries. You can look at the new test_triso to see how the class and function are used.

@wbinventor Note that I've held off on adding a test for the Python plotting capabilities mostly due to the fact that there's no good way to hash a PNG (and writing raw RGBA data from matplotlib was broken apparently). The other reason I'm deferring is that we should really decide what we want to do with respect to unit testing.

[wbinventor]

I will review this later today, though it might be nice if @smharper can help out, particularly with regards to openmc.model. Thanks for holding off on the ID-related changes to __deepcopy__, we can cogitate on that together a bit more first.

As for the testing your new plotting methods, I feel your pain with matplotlib. I'm not sure how much you looked into running diffs on matplotlib-generated figures, but it is worse than you might imagine. The images generated on different machines with the exact same install of matplotlib (and the exact same rcparams settings) may generate figures of slightly different width x height specs, even if you try to specify them manually. Needless to say, this makes a diff between a test and reference figure difficult.

All of our tests for the plotting module in OpenMOC by generating an RGB histogram of the pixel in a test/reference plot and then diff'ing the histogram (as recommended on Stack Overflow) as illustrated here. This hacky solution is known to miss small but perhaps meaningful deviations in figures (link), but will at least catch any medium to large errors.

[wbinventor]

I'm curious - is this line included in the default implementation? Seeing this is now sparking my memory of one of the reasons I went about implementing __deepcopy__ methods back in the day - to boost performance by eliminating redundant copies of objects in CSG trees (i.e., this line of code). I want to say that this is not included in the default implementation, but I'm not sure. Regardless, I don't think it matters for the Cross* and Aggregate* classes anyway.

[wbinventor]

I also may be wrong, the memo may be used in this way by default.

[paulromano]

Yes, as far as I can tell memo is used this way by default. Here's proof:

In [1]: import openmc, copy

In [2]: c = openmc.Cell()

In [3]: l = [c, c]

In [4]: l_copy = copy.deepcopy(l)

In [5]: [id(x) for x in l_copy]
Out[5]: [140129480509648, 140129480509648]

In [6]: memo = {id(c): 'See Will, I told you so'}

In [7]: l_copy2 = copy.deepcopy(l, memo)

In [8]: l_copy2
Out[8]: ['See Will, I told you so', 'See Will, I told you so']

[paulromano]

@wbinventor I only looked into diffing plots enough to know that it is painful, not to what degree :) Good to know! And sounds like we can learn from your experience when the time comes.

[smharper]

This can sample particles that are split by the reflective boundary condition. These should be changed to something like

            x = random.uniform(-0.5+radii[-1], 0.5-radii[-1])

Or you could move the reflective BCs outwards by radii[-1]

[smharper]

@paulromano, this implementation has all of the TRISO surfaces on the same universe level. I would really recommend using a two-level approach where the upper universe only has one surface and cell per TRISO. That cell is filled with a lower universe containing the rest of the surfaces.

There are two big advantages to this method: The distance_to_boundary calculation only has to check one surface per TRISO at that coordinate level. It also removes redundant surfaces from the geometry.xml which will significantly reduce the time required to read geometry.xml (reading the geometry can easily take several minutes).

I have a rough example implementation in my repo. The test you added runs twice as many neutrons/s with these changes. I suspect the speedup will be even larger for higher packing fractions.

[paulromano]

@smharper Nice idea with the multiple universes! Though to be clear, even with the current scheme there should still only be one surface per TRISO that is checked during distance_to_boundary. That being said, there is certainly an input reduction and performance advantage to having a single universe for many TRISOs though, so I will adopt your idea.

[smharper]

Hmm, I see now that you're right about distance_to_boundary. I wonder what causes the 2x performance difference. I doubt it would be find_cell. I'm tempted to profile the runtime.

[paulromano]

Well, since the single universe (and cells within) for TRISOs gets reused as a particle is tracking through, it should be a little more cache-friendly all around.

[smharper]

I did profile the code. It turns out that find_cell really is a bottleneck. With the 1-level approach, find_cell takes 67% of the runtime. With 2-level, it takes 37%. I bet this is because of the call from cross_lattice since that call doesn't use a reduced search list.

[paulromano]

@smharper Your comments have been addressed. I also added a commit fixing an issue with Material.add_nuclide whereby if you add two nuclides with the same name but different xs, the first one gets overwritten.

[smharper]

int -> Real

[smharper]

I'm happy. @wbinventor, have all of your concerns been addressed?

[wbinventor]

This ship is ready to sail!

[paulromano]

Thanks guys! Can we also sail my one-liner in #672?

[smharper]

Shesh, you give 'em an inch...