-
Notifications
You must be signed in to change notification settings - Fork 33
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
API Provide generalized detectors in serpentTools.detectors #341
Merged
drewejohnson
merged 20 commits into
CORE-GATECH-GROUP:develop
from
drewejohnson:detector-cleanup
Oct 4, 2019
Merged
API Provide generalized detectors in serpentTools.detectors #341
drewejohnson
merged 20 commits into
CORE-GATECH-GROUP:develop
from
drewejohnson:detector-cleanup
Oct 4, 2019
Conversation
This file contains 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
No need to define extra strings that facilitate the conversion, when a simple "{}_{}".format will suffice, or x + "_" + y. The latter is used in this commit
Heavily refactored the detector objects. A summary of the changes is presented here, with justifications to follow: - Detector scores are removed - closing GH Issue CORE-GATECH-GROUP#327 - Detector.fromTallyBins classmethod replaces creating a detector and then adding tally data in separate method - Detector objects are more flexible, relying on properties to access and modify tallies, errors, and grids - DetectorBase is removed. Detector is a fully functional class - All detectors can be imported from serpentTools.detectors and are included in the main API, e.g. serpentTools.Detector - Importing detectors from serpentTools.objects.detectors is deprecated, favoring serpentTools.detectors - Heavily re-document the detector interface. The main documentation is now explicitly written, with no programmatic doc-string building. That got ugly to read. NON-FIXES - Samplers need to be fixed and have not been modified - No checks are currently performed on the shape of incoming tally and error data, e.g. they could potentially be of different shapes The primary justification for this change was to allow the detectors more flexibility over how their data was provided. The previous method relied exclusively on the addTallyData method to perform the reshaping. Now, the user can create a Detector already knowing the, or without knowing, the tally data. Setting tallies and errors are now done through properties, ensuring that newly set data is of identical shape. This goes to resolve GH issue CORE-GATECH-GROUP#337 by allowing the user more natural control over tally data. The new pytest-style test file tests/test_detectors.py modifies the tally and error data in-place to emulate some scaling. Work still needs to be done to fix the sampler module, as the sampled detectors rely upon the now removed DetectorBase
If allTallies is None, then there is no is nothing to be plotted. Previously this was a SerpentToolsException
Similar to changes introduced in 5c9c46f, the SampledDetector is now more like a standalone object. It can be created directly by passing allTallies and allErrors, or with an iterable of similar detectors through SampledDetector.fromDetectors. The implementation is a lot cleaner, relying less on hidden methods for finalizing and computing errors. The propagation of uncertainty is done during the init method, as is the averaging of the tally data. Additionally, a new attribute is provided: deviation. This attribute provides the standard deviation in the expected tally data.
Returns reshaped tally and error data, as well as underlying indexes. Pulled out from Detector.fromTallyBins for transparency. Docstring includes a simple example
The grids can be provided directly at init, or can be set after the fact. x, y, and z are all exposed as properties of the CartesianDetector with appropriate setters. Rename _getGrid -> _getPlotGrid for clarity.
Z grid, centers of hexagons [COORDS], pitch, and hexType are now arguments that can be passed to the init method, and modified through properties. tests/test_detectors::testHexagonalDetector has been added to test the variety of ways one can build and modify a HexagonalDetector
Use meshedBinData fixture for testCartesianDetector as well
Unit test added to ensure that score column is not allowed Related: GH CORE-GATECH-GROUP#327
Same goes to SampledDetectors More concise and harder to break due to immutability of tuples. Detector.indexes now a property that checks that tallies and/or errors are set up and the number of items corresponds to the dimensionality of the data. Using this tuple, the plot routines are modified to ensure that tally data and indexes are set up prior to any slicing and plotting.
Use kwargs.setdefault when appropriate to give our preferred settings, but also defer to the user's values. Use some error checking at the top of the functions to reduce complicated logic
Also remove swap files and tag files
drewejohnson
added
documentation
API - Compatible
Changes to our API that require no user actions
API - Incompatible
Incompatible changes to our API that require user actions
labels
Oct 3, 2019
- Remove all super calls in favor of directly calling the parent method - Place Mappings in compatibility file This can safely be reverted as a part of GH Issue CORE-GATECH-GROUP#328, when python 2 support is removed
DanKotlyar
approved these changes
Oct 4, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
API - Compatible
Changes to our API that require no user actions
API - Incompatible
Incompatible changes to our API that require user actions
documentation
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.
This PR introduces a heavy re-writing of the detector classes, while seeking to retain much of the previous API. A summary of the changes are provided here
fromTallyBins
class methodThe above changes are also inherited in our subclasses CartesianDetector, HexagonalDetector, SphericalDetector, and CylindricalDetector.
Through the use of properties and some better logic in the plot routines, modifying tally data for plotting is much easier. This closes #337, at least for detectors. Working towards #335 will hopefully fully resolve the spirit of that issue.
The bulk of the changes are in
serpentTools/detectors.py
andserpentTools/samples/detector.py
Indexes as tuple
The goal for Detector.index is to easily and programatically determine which dimension in tallies and errors corresponds to a specific bin. This makes an OrderedDictionary not well suited, as the only way to retrieve the axis for a specific bin, e.g. energy, is to iterate over all keys until you've hit energy. Secondly, storing an array of the "unique" values, which was really just
numpy.arange(nBins)
, didn't make a ton of sense. We already know how many bins a specific bin type takes up by looking at that dimension in the tally data.Philosophical justification
The main reason I wanted to do this follows arguments in #335. By generalizing different classes, we are less dependent on how we get the data. This can be Serpent matlab files or some other intermediate storage that we or other users decide to implement.