Open
Conversation
ajkunen
requested changes
Jun 14, 2019
Collaborator
ajkunen
left a comment
There was a problem hiding this comment.
This looks really good, just a few things would be great:
- I'm getting a number of compiler warnings, mostly related to the need to convert some initializer lists to use double curly braces (for subobject initialization)
- It would be nice if the --compute_errors flag was added to the --help output
- It would be nice if there was some mention of it in the README file, with the note about running with sigs=0
- This doesn't compile on GPU machines. Two options: either make it work on the GPU (this may be difficult) or make it a optional feature that can be controlled via CMake (like -DENABLE_COMPUTE_ERRORS). That way we can turn it off (and not compile it) for GPU builds
…ge message. 3) added something about the error code to the README
Author
|
I believe I have addressed all the issues in the review. I made some changes so that the errors work on gpu builds. Basically I added policies for ErrorNorms that are sequential when GPU builds are enabled. I built and tested these on lassen. Please let me know if there are more corrections and/or additions to make. |
Collaborator
|
@kkhemc Thanks! I'll try and take a look at this over the weekend or next week. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here are some additions to Kripke that allows it to compute error norms for the type "i" problems, i.e. no scattering. You would run kripke with something like
kripke.exe --sigs 0,0,0 --zones 200,400,200 --quad 8:4 --niters 1 --compute_errors
If this is useful, we could add diagnostics comparing to the angle integrated total fluxes for the "type ii" problems. The added code has the necessary benchmark data, we would just need to add the specific diagnostic calculations.