Update to Quadrature #296
Update to Quadrature #296
Conversation
|
Cool stuff @geogunow! I'll look this over later today. The test suite appears to have failed, any idea why? Also, would you mind putting in one or more new tests of the various |
|
Also, two other things:
|
|
@wbinventor @samuelshaner this PR is ready for review |
|
Great, I'll review this tonight. |
| @@ -748,6 +748,11 @@ void CPUSolver::tallyScalarFlux(segment* curr_segment, int azim_index, | |||
| FP_PRECISION* sigma_t = curr_segment->_material->getSigmaT(); | |||
| FP_PRECISION delta_psi, exponential; | |||
|
|
|||
| /* Extract weights for all polar angles */ | |||
| FP_PRECISION weights[_num_polar]; | |||
There was a problem hiding this comment.
So we are using allocating weights on the stack each and every time CPUSolver::tallyScalarFlux(...) is called? Might it not be better to call _quad->getWeight(...) directly within the nested loop over energies and groups? If you inline this Quadrature::getWeight() I doubt that this would lead to a performance hit, and would reduce the code in this method by a few lines.
There was a problem hiding this comment.
If I inline the Quadrature::getWeight(...) function I would probably need to get rid of the error checking conditionals. Do you think I should do that?
|
The documentation for the azim spacing in |
|
It looks like the inputs in the profile directory have not been updated. Could you update those? |
| exponential = _exp_evaluator->computeExponential(sigma_t[e] * length, p); | ||
| delta_psi = (track_flux(p,e)-_reduced_sources(fsr_id,e)) * exponential; | ||
| fsr_flux[e] += delta_psi * _polar_weights(azim_index,p); | ||
| fsr_flux[e] += delta_psi * _quadrature->getWeightInline(azim_index, p); |
There was a problem hiding this comment.
I like how clean this is and that it avoids making a weights pointer attribute in the Solver class!
|
The |
| * @endcode | ||
| * | ||
| * @param weights The polar weights | ||
| * @param num_azim_times_polar the total number of angles (azimuthal x polar) |
There was a problem hiding this comment.
Can you note that this is just the total number of angles in one octant?
|
@samuelshaner I don't think I ever failed the |
|
You updated the results for |
|
@samuelshaner you're right. It did change and the new plot looks incorrect. It is corrected now. However, I was right too - I never failed the |
|
@geogunow our plotting tests all use a thresholded comparison of a 3D RGB histogram of the pixels in the reference and generated images (link). This (or something similar) is necessary since the size of the matplotlib images generated on different machines is inconsistent. The threshold used is completely arbitrary right now, and is probably too high to catch just the few pixel difference between the quadrature plots with 3 vs. 4 points. It should be large enough, however, to catch blatant discrepancies in plots, but it is certainly not good enough to catch everything. You can always tighten the tolerance for the quadrature test if you want to ensure this doesn't happen again. |
|
I just ran the |
|
I don't think the distance metric for our plotting tests needs to be fixed in this PR. But certainly making note of it as an issue as @samuelshaner proposes sounds like reasonable. |
|
@wbinventor @samuelshaner The problem has been fixed for this PR. I just posted an issue about the test suite checking .png images. |
|
This looks up to par and ready to merge to me. |
|
Thanks again for syncing up the quadrature @geogunow! |
In this PR I update the quadrature used in OpenMOC to better generalize. In particular the following major changes were made:
PolarQuadclass has been changed toQuadratureto imply generalizations outside of just the polar angleQuadratureclass. This includes azimuthal weights and azimuthal spacings.Quadratureclass is no longer owned by instances of theSolverclass - but rather instances of theTrackGeneratorclassQuadratureclassTrackGeneratorhas been changed so that all angles in [0, 2*pi] are stored. This improves the readability so that the definition of_num_azimis consistent. Although this does mean storing duplicate data, there should be no performance hit.This PR is the first step to syncing OpenMOC with ClosedMOC's 3D-MOC solver.