Fix energy bin sampling in source #941
Conversation
|
Unreversing |
|
@paulromano Yes. And that only matters if the tally's energy/energout filters bins dont match up with the group structure; if they do match then the particle's group is used to set the filter bin. @brbass The energy bin indexing is set up as follows:
After digging deeper and reminding myself of the above, I think instead this PR might want to focus on fixing source.F90 while minimallly perturbing the rest of the source that implements the above energies. That code looks to be simply incorrect (even if there wasnt confusion about the ordering). In addition to the errors you've pointed out, the code should probably be checking for sampled energies outside the bounds of the problem and erroring if found. |
|
I agree -- easiest thing to do right now is to just fix source.F90 and leave everything else as is. |
brbass
changed the title
|
I reversed the energy groups back to their original format. Since the binary search used to find the energy group in source.F90 requires that the data be ordered from smallest to largest, I added the rev_energy_bins to mgxs_header.F90 and correctly initialized it in input_xml.F90. This requires that the energy group be reversed after the search. Finally, I changed the tally_filter_energy.F90 to use the rev_energy_bins array instead of reversing energy_bins. |
|
As a side note, it might be a good idea to add a multigroup, fixed-source problem with an energy-dependent source to the tests. Since the incorrect code didn't raise any flags, I'd guess there aren't any tests that would fail if the source particle was always sampled in the group with index one. |
|
You're absolutely right on that one. And perhaps some fixed source
benchmark problems to the benchmark repo. Would you like to handle the
fixed source MG test, or should that be separate?
…On Dec 11, 2017 5:46 AM, "Brody Bassett" ***@***.***> wrote:
As a side note, it might be a good idea to add a multigroup, fixed-source
problem with an energy-dependent source to the tests. Since the incorrect
code didn't raise any flags, I'd guess there aren't any tests that would
fail if the source particle was always sampled in the group with index one.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#941 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA_TM61zs_ltlVJujEydY4beNzAHNV_Hks5s_QgcgaJpZM4Q8S9t>
.
|
|
@nelsonag I could add the fixed-source MG test sometime in the next few weeks. For the purposes of testing the code we've changed here, a three-group infinite medium problem with an energy-dependent source and an analytic solution would be a good sanity check. I'm not familiar enough with the existing tests to say if others are needed. |
|
@brbass The changes so far look good. We might as well wrap in the checking the MG-sampled source to make sure the energy sampled is within the range of the multi-group structure. To get the high-end energy we simply need to set the The maximum energy will then be checked by the SourceDistribution % sample method. I personally vote for the latter; i suspect its more likely for this to be an issue in MG mode than CE and therefore dont think the additional if-then is necessary for every source sample in both modes. Thoughts? |
|
In terms of code simplicity, putting the minimum energy check inside sample() in SourceDistribution would be preferable, since in the sample_external() function of source.F90 we would then just do the binary search directly without adding an additional check. This would also mean we wouldn't need to add the nuclide header to source.F90. My issue with checking in the source.F90 code for the minimum energy would be that we would have to either (1) decide to throw a fatal exception if the energy was below the minimum energy boundary or (2) put in another "do" loop for resampling the external source if the energy was below the minimum. It seems like the first option would be overly restrictive for continuous distributions, since the user may wish to specify a distribution and then only accept energies inside the MG energy bounds. The second option would be pretty clunky, since there's already a rejection mechanism built into the SourceDistribution code. Would adding a check for minimum energy in the SourceDistribution break anything in continuous energy land? As an aside, is there some reason why the discrete source is checked every time for incompatible energies in source_header.F90? Unless the discrete sources are changing after reading the input, it would be a lot more efficient to check the sources for compatible energies just after the sources and cross sections are read in. |
|
All very good points @brbass. I agree with you that it is probably cleaner to have that check in Regarding the discrete distribution check, the reason it's probably there is because at the time the source distributions are read in (as part of settings.xml), the cross sections haven't been loaded yet, so the check has to be deferred until afterwards, but having a check on the source at the time the cross sections are loaded might appear a little awkward, so we probably found it a little more logical to have it in source_header.F90 |
…gy bin search in source.
…up min and max energies and removed check for these after samping the source.
|
I'm going to go ahead and merge this as it looks fine. @brbass If you end up writing some tests for this, you can submit a separate PR. |
The algorithm to find the appropriate energy group for a sampled particle in source.F90 assumes the energy bounds are ordered from lowest to highest. The current implementation reverses the energy bins to be from highest to lowest in input_xml.F90.
This change reverts the energy groups to their original order, from lowest to highest. This has a side effect of aligning the energy bins with the tally bins, which previously were reversed.
After the change, test_mgxs_library_ce_to_mg.py fails. It looks like the the difference is probably due to a change in order of some random operation on the flipped array of energy bins. As I increase the number of particles, the old and new results converge to the same eigenvalue and same cross sections. I went ahead and updated the results of the test for now.