Fix numerical diffusion in uniformMesh converter #992
Conversation
Mapping anything from a non-uniform assembly to a uniform assembly and then back will cause numerical diffusion of that value. This could be a block parameter (like mgFlux, detailedDpa, etc.) or number densities. This commit deals with the issue of block parameters being mapped back and forth. The number density issue has already been addressed for the nonUniformAssemFlags case, but it seems to still be present for the detailedAxialExpansion case.
|
Not requesting review yet, just tagging a few interested parties @keckler @Nebbychadnezzar |
The power isn't used for any real calculations, but it's necessary to map power onto the uniform mesh reactor because some downstream physics solvers want to check the energy balance before proceeding. The "energy balance" is checked by comparing the sum of b.p.power to the power specified by the settings input.
Extend NeutronicsUniformMeshConverter class for gamma cases and flux recon cases.
Add a test to verify that reactor params are mapped correctly by global flux executer when nonUniformAssems are used. Needed to fix a bug to get reactor params to map.
|
@Nebbychadnezzar can't select you as a reviewer but I wanted to get your eyes on this as well. |
There was a problem hiding this comment.
I like it. I left a handful of comments and suggestions, but I don't see anything blatantly wrong.
After this is merged, though, there is still work to do in our internal plugins to move the application/calculation of cumulative parameters outside of the geometry conversion entry/exit. For instance, I can say for certain that detailedDpaRate is calculated right now when the geometry is still converted. Likely the same for many of the cumulative parameters. So we'll have to hunt all of those down and update them, if necessary.
For cumulative parameters that are being calculated within the framework itself, I would argue it might make sense to update those as part of this PR (if any of them exist).
| assignInBlueprints = "assign in blueprints" | ||
| retainOnReplacement = "retain on replacement" | ||
| pinQuantities = "pinQuantities" | ||
| fluxQuantities = "fluxQuantities" | ||
| multiGroupQuantities = "multi-group quantities" | ||
| neutronics = "neutronics" | ||
| gamma = "gamma" |
There was a problem hiding this comment.
There are some parameters that you've given this tag which, on the face of it, don't look like they are associated with gamma. I'm thinking specifically things like powerNeutron, pdensNeutron, etc.
My guess is that they have the gamma tag because they are calculated in the gamma transport step.
To me, this is a little bit confusing perhaps for someone who might come along later. Possibly a description here, or a change of the category name, might be useful.
There was a problem hiding this comment.
Yes, it is confusing. If only neutron transport is run without gamma, then the power and pdens parameters are populated. Once gamma is run, then powerGamma and powerNeutron are populated.
I think they have to stay in this category, so maybe I'll include a comment or update the parameter description.
There was a problem hiding this comment.
I would love a docstring or description in the /docs/ somewhere that explains to people what to use these two new categories for.
I mean, I see your description above about going into/out of the uniform mesh. But we want people who come to ARMI in a year to be able to understand this, right?
There was a problem hiding this comment.
I added some docstrings to describe most of the parameter categories in this class.
| UniformMeshGeometryConverter._applyCachedParamValues( | ||
| destBlock, blockParamNames, cachedParams | ||
| ) | ||
|
|
There was a problem hiding this comment.
My understanding of this change is as follows:
- Before, we were clearing all of the block parameters on the new, converted assembly, while storing some of them temporarily in a "cache". Then, we were mapping the specified parameters to the new assembly, and the cached parameters are then used to put the other parameters back onto the blocks.
- Now, we just avoid clearing the parameters in the first place, so that the caching of parameters isn't needed for anything. All parameters that are not specified specifically to be mapped are just left as their initial values?
This seems like potentially a source of great speedup. But it makes me wonder, are we losing something by changing that flow? It seems to me that an expensive step, like clearing all the parameters in the first place, must have been done for a good reason. But I cannot see why that reason might have gone away now...
There was a problem hiding this comment.
My understanding of the caching is basically the same as yours.
As for whether the caching is necessary, I would break it down into the two directions:
- Non-uniform source, uniform destination assembly: When the destination assembly is the uniform assembly, it's being created from the source assembly, so it shouldn't have any pre-existing parameters that need to be cached.
- Uniform source, non-uniform destination: When the destination assembly is the non-uniform assembly, parameter caching would ensure that we don't erase any of the parameters in
blockParamNameson the non-uniform assembly by overwriting with null values from the source assembly. But, if we have defined theblockParamNamesto be mapped back correctly, then we should only be mapping back parameters that have been set on the uniform assembly. If thissetAssemblyStateFromOverlapsis trying to map back parameters that haven't been set, then either the_setParamsToUpdatewas not defined correctly or maybe some analysis performed on the uniform reactor failed._applyCachedParamValueswould paper over that error by just retaining whatever value the non-uniform assembly already had for that parameter, but this would create a "silent" bug that would be very difficult to track down (e.g., you have ab.p.powerin the database, but you have no way of verifying whether it was mapped back correctly or just retained by caching).
That's my take on it, but it's complicated and I'm definitely curious to hear the opinions of others. I think the short summary is that parameter caching would not be performing any detectable function when everything is working correctly. It would only act when there is a bug in the code or an error during execution, and it would effectively cover up the error without giving any warning or error message.
There was a problem hiding this comment.
My major concern with preserving the old, caching solution is backwards compatibility for our downstream projects.
But it doesn't appear well enough documented that I'm thrilled with having to support it forever. Michael's above description matches what I see in the code. But the original developer could have saved us, collectively several hours of digging if it had been better documented.
Let's do better!
I agree with this sentiment. I just scanned through the framework parameters and couldn't find any which are cumulative that would also be mapped in the first place. There are some things in the So we're probably good there! |
Parameter categories that need to be mapped for gamma might include multigroup quantities (depending on the gamma source production method selected). Neutronics needs to avoid mapping back cumulative parameters--even though they aren't mapped in, they can be present on the uniform assembly because the blocks were deepcopy'd from the original non-uniform assembly.
This reverts commit 71d69c3.
This reverts commit f6dde5e.
|
I pulled this PR into my ARMI app, reran some extensive calculations, and then looked at the calculated DPAs. I can confirm that this PR resolves the issues I was previously having with DPA possibly decreasing as time increases. Obviously that was non-physical, and these changes resolve that such that we are now getting physically-possible results for DPA. I can also add another data point to add confidence to this change. The model that I ran is a benchmark of real reactor operations, and I was previously getting anomalously low reactivities in later cycles. I had suspected that this was due to the numerical diffusion of the uniform mesh generator, and it turns out that with this fix, core reactivities line up much more closely than before. So in my book, the changes in this PR are a resounding success. I approve! |
albeanth
requested review from
Nebbychadnezzar
and removed request for
albeanth
So... above you were concerned that "there is still work to do in our internal plugins to move the application/calculation of cumulative parameters outside of the geometry conversion entry/exit". Are you concerns with that over? |
Yes, I am sufficiently convinced after both looking though our internal code and testing this PR that we are in the clear on that concern. |
|
I'm not sure why, but I'm getting an error in JOYO that I thought I fixed previously. I looked to make sure my previous changes were still there and they are. I don't know if any of you have seen this error before: File "\albert\home\mhuang\site-packages\nala_main_.py", line 26, in main |
|
@LMikeH I'll message you offline. |
There was a problem hiding this comment.
I think this is a good solution, in that it solves a semi-complicated problem.
The solution is as complicated as the problem. But sometimes there is just no way around that.
As long as we are all convinced by the documentation we've put in place to explain it to future generations, I'm happy.
Description
Mapping anything from a non-uniform assembly to a uniform assembly and then back will cause numerical diffusion of that value. This could be a block parameter (like mgFlux, detailedDpa, etc.) or number densities. This has been observed for neutron flux while using the
nonUniformAssemFlagssetting on control rods, and fordetailedDpawhen using thedetailedAxialExpansionsetting.This commit deals with the issue of block parameters being mapped back and forth. The number density issue has already been addressed for the nonUniformAssemFlags case, but it seems to still be present for the detailedAxialExpansion case.
The selection of which parameters to map is very much up for discussion. Please review the categories that have been set for
REACTOR_PARAMS_TO_MAPandBLOCK_PARAMS_TO_MAP, and the_setParamsToUpdatemethod for both neutron and gamma transport. Please leave comments on any parameters you think are missing or should be changed. My approach to these selections is summarized below:Checklist
doc/release/0.X.rst) are up-to-date with any bug fixes or new features.docfolder.setup.py.