Restore the prune function to Ordinate_Space. #477
Conversation
This is overridden by Galerkin_Ordinate_Space and this changes the solution slightly in one of the Capsaicin benchmarks.
Codecov Report
@@ Coverage Diff @@
## develop #477 +/- ##
=========================================
+ Coverage 91.8% 91.8% +<.1%
=========================================
Files 375 375
Lines 17472 17476 +4
=========================================
+ Hits 16045 16054 +9
+ Misses 1427 1422 -5 |
|
@kgbudge There were testing issues on ccscs2 (not your fault). I'll restart the tests. |
There was a problem hiding this comment.
Overall this looks fine. However, I would like to see a unit test addition that covers the new code in Ordinate_Space.hh and a new test case that covers the code block that can be activated by prune()==false in Ordinate_Space.cc (see review comments for details).
src/quadrature/Ordinate_Space.cc
Outdated
| // member function prune() always returns true? | ||
| // if (l <= Lmax || !prune()) { | ||
| if (l <= Lmax) { | ||
| if (l <= Lmax || !prune()) { | ||
| if (l > Lmax) { | ||
| Lmax = l; |
There was a problem hiding this comment.
Can you provide a test case that covers this branch for this block of code?
if (l > Lmax) {
Lmax = l;
moments_per_order_.resize(Lmax + 1, 0U);
}
This seems like a corner case and I don't understand what conditions would lead us here.
|
@kgbudge It looks like the added function |
The prune function of Ordinate_Space always returns true, and on this basis it was removed from the class. However, this is a virtual function that is nontrivially overridden by Galerkin_Ordinate_Space, and its removal changes the solution slightly in one of the Capsaicin benchmarks.
The difference is quite small, but until I can better recall why this was done in the first place, I suggest we restore the Ordinate_Space::prune function and restore the if-test using prune.
Status