-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/multigroup part9 #17
Conversation
python::module submodule = module.def_submodule( | ||
|
||
"depletion", | ||
"Depeltion NDI records and subrecords" |
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.
Depletion
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.
Changed in fix/review-part6
chunk = Multiplicities( reaction = 16, products = [ 1, 92234 ], | ||
multiplicities = [ 2, 1 ] ) | ||
|
||
verify_chunk( self, chunk ) |
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 chunk_string also be used here?
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.
This is used inside the verify_chunk function, but since it is not a keyword record (it is a subrecord) we cannot create this subrecord from a string on the Python side.
self.assertEqual( self.chunk_typed_string, chunk.to_string() ) | ||
|
||
# verify the record | ||
self.assertEqual( 'rprod_all', chunk.keyword ) |
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.
Isn't the type (all) stored somewhere? Should we check that?
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.
Yes. the type is stored, and I forgot to test it. That has been changed in fix/review-part6
@@ -5,6 +5,9 @@ MultigroupTable() : | |||
metadata_(), primary_structure_(), | |||
velocities_(), weights_(), total_(), xs_(), scattering_(), | |||
release_(), primary_heating_(), primary_kerma_(), | |||
product_multiplicities_all_( depletion::ReactionMultiplicityType::All ), |
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.
Just for my understanding: all the others are empty parenthesis (default constructor), why do these have something in them?
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 these records, we need to know the subtype to construct the full keyword name. This is the same for the nubar records that are introduced in part10.
|
||
if ( this->metadata_.numberReactions().has_value() ) { | ||
|
||
readRecord( this->product_multiplicities_all_, iter, end, |
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.
Will this be fine is the number of reactions is 0? Or would that never happen?
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.
Can the number of reactions actually be zero?
ReactionMultiplicities() : ReactionMultiplicities( "" ) {} | ||
|
||
/** | ||
* @brief Default constructor |
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.
Not default.
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.
Changed in fix/review-part6
@@ -0,0 +1,8 @@ | |||
static std::string getPostFix( const ReactionMultiplicityType& type ) { | |||
|
|||
return type == ReactionMultiplicityType::All |
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.
What happens if there isn't a postfix or if it isn't all, few, or rmo?
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.
This has been removed in part10
std::vector< int > data; | ||
while ( reactions-- ) { | ||
|
||
data.push_back( njoy::tools::disco::FreeFormatInteger::read< int >( iter, end ) ); |
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.
Would you mind adding a comment as to what data this should be reading? Like, the first is the... MT? Second the number of products?
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.
Changed in fix/review-part6
* | ||
* The following verification tests are performed: | ||
* - there is at least one reaction | ||
* - the number of groups for each reaction is the same |
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.
Not applicable
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.
Changed in fix/review-part6
CHECK( 2 == chunk.reactions()[1].multiplicities()[0] ); | ||
CHECK( 1 == chunk.reactions()[1].multiplicities()[1] ); | ||
|
||
auto multiplicities = chunk.reaction( 2 ); |
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.
Did you mean to use multiplicities rather than chunk.reaction( 2 ) below this?
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.
I removed the line in fix/review-part6
This adds the reaction product multiplicity records.