-
Notifications
You must be signed in to change notification settings - Fork 1
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 part10 #19
Conversation
|
||
module, | ||
"FissionNeutronProduction", | ||
"A fission neutron production record for multigroup neutron and photon data" |
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.
Do you mind removing "and photon data" for now? For multigroup we only create photons with a production matrix that's included with the scattering matrix. I don't want people to expect the wrong thing.
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.
COnsider it done. See fix/review-part6.
"Initialise the record\n\n" | ||
"Arguments:\n" | ||
" self the record\n" | ||
" values the fission neutron production values" |
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.
Don't forget "type"
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.
"record\n\n" | ||
"Arguments:\n" | ||
" string the string representing the record\n" | ||
" number the fission neutron multiplicity values" |
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.
Why call this "number"? Especially since it's multiple values.
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.
Looking further down, should this be the number of groups?
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. Changed in fix/review-part6.
"record\n\n" | ||
"Arguments:\n" | ||
" string the string representing the record\n" | ||
" number the fission neutron production values" |
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.
Same as before with "number"
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.
Number of groups?
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. Changed in fix/review-part6.
|
||
module, | ||
"FissionNeutronSpectrumMatrix", | ||
"A fission neutron spectrum matrix record for multigroup neutron and photon data" |
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.
Same here about photon.
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 changed it everywhere. Changed in fix/review-part6.
FissionNeutronSpectrumMatrix() : RealListRecord( base::Keyword( "chi_mat" ) ) {} | ||
|
||
/** | ||
* @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.
Mention type?
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 value | ||
* |
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.
Mention the "check for matrix" test.
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.
|
||
GIVEN( "invalid data for a FissionNeutronSpectrumMatrix instance" ) { | ||
|
||
WHEN( "the number of weight values is insufficient" ) { |
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 weights.
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.
Also, we should test the "matrix not square" bit too.
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.
Damn you copy/paste!
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. And added square test.
FissionNeutronSpectrumVector() : RealListRecord( base::Keyword( "chi_vec" ) ) {} | ||
|
||
/** | ||
* @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.
Type?
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.
/** | ||
* @brief Constructor | ||
* | ||
* @param[in] values the fission neutron spectrum vector values |
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.
Type?
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.
This adds the last few remaining records for the multigroup neutron and photon files:
The only few remaining things to finalise are the constructor of the MultigroupTable to use these records (the PR was getting big) and maybe some consolidation of records into logical blocks to make it easier for the user (e.g. putting all fission related records in their own class as we did with the metadata).