-
Notifications
You must be signed in to change notification settings - Fork 11
Material refactor plus integration tests #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Material refactor plus integration tests #17
Conversation
This pull request moves functionality for creating materials and calculating stopping power into neucbot/material.py. As part of this refactoring, the stopping power calculations now use binary search (using bisect.bisect) rather than sequential search for faster computations on average (logarithmic time rather than linear time).
As some of the implementation details for neucBOT change under the hood (e.g. the use of binary search rather than sequential searching), there needs to be a comprehensive way beyond unit tests to ensure that no regressions are introduced. To do this, outputs files were generated for the following three commands on the current master branch: $ python3 ./neucbot.py -m Materials/Acrylic.dat -c Chains/Th232Chain.dat $ python3 ./neucbot.py -m Materials/Acrylic.dat -l AlphaLists/Rn220Alphas.dat $ python3 ./neucbot.py -m Materials/Acrylic.dat -l AlphaLists/Bi212Alphas.dat These output files will serve as integration tests. As more neucBOT refactoring and enhancements occur, they should not change the expected outputs of these commands. In a subsequent commit, these tests will be required to pass in all Github PRs before merging.
88869a4 to
8cc6c75
Compare
| mater = material(element.symbol,A_i,frac*abund/100.) | ||
| mat_comp.append(mater) | ||
| else: | ||
| mater = material(element.symbol,A,frac) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have a frac / 100. as well? Seems like a chance for inconsistency between the two cases where A is specified as 0 or as a fixed isotope.
| # if the alpha energy is too high for the list, use the highest energy on the list | ||
| if not sp_found: | ||
| sp_alpha = sp_last | ||
| sp_total += sp_alpha * mat_comp_reduced[mat]/100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding, why do we have the /100 here? If this is to handle the fact that mat_comp_reduced[mat] is a fractional value between 0 and 100, it looks like that is already partially handled above on line 91 (but not on 94).
If that is the case, then this conversion to a float between 0 and 1 is handled in the new neucbot/material.py file here and here.
The stopping power calculation for a material at a given alpha energy level was incorrectly copied/modified when being moved from neucbot.py to neucbot/material.py, and this was caught by the integration tests added in an earlier commit. The unit tests have been modified accordingly and the integration tests are almost passing, they are off by ~1e-16.
This pull request factors material-related code (including the calculation of stopping powers) into its own
neucbot/material.pyfile, and also adds integration tests to ensure that any change in functionality does not impact the expected output of neucBOT.More details can be found in the commit messages.