-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add neutron inelastic process #1187
Conversation
@sethrj, @amandalund : this MR is just to pick up most of #1150 codes (sorry for losing all review comments with this new MR) - will add remaining codes later as they are not necessary pieces for initial code base at the moment. Only new addition to the earlier MR are for nucleon-nucloen cross sections in 1) |
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.
Thanks @whokion, this looks great!
auto NeutronInelasticModel::get_channel_xs(ChannelId ch_id) | ||
-> ChannelXsData const& | ||
{ | ||
CELER_EXPECT(ch_id); |
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.
Maybe check that ch_id
is less than the array size?
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 mean to check ch_id < NeutronInelasticScalars::num_channels()?
/*! | ||
* Calculate neutron inelastic cross sections from NeutronInelasticData | ||
*/ | ||
class NeutronInelasticMicroXsCalculator |
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 we ever decide on the best way to reduce the duplicate code for the neutron micro xs calculators?
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.
Kind of, but not yet. There is another one (capture process) which may look very similar, but may have some variations if isotope dependent xs are added. So, let's leave this duplication for now and then consolidate codes later (may need your help on 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.
@whokion The data and implementation seem identical to NeutronElasticData (setting aside capture for now). Would it be OK to define:
template<Ownership W, MemSpace M>
struct NeutronXsData
{
//! Microscopic (element) cross section data (G4PARTICLEXS/neutron/elZ)
ElementItems<GenericGridData> micro_xs;
// Backend data
Items<real_type> reals;
// (plus copy constructor, bool)
};
and then replace the micro_xs
and (if needed) reals
in both NeutronElasticData
and NeutronInelasticData
with an instance of that class? E.g.
template<Ownership W, MemSpace M>
struct NeutronElasticData
{
// ...
NeutronXsData<W, M> xs;
// ...
};
Then NeutronElasticMicroXsCalculator
could be renamed NeutronMicroXsCalculator
and used for both elastic and inelastic. Is there anything I'm overlooking that would make this complicated?
@sethrj I resolved conflicts and updated codes reflecting @amandalund's review comments, then rebased the local neutron-inelastic with origin/develop (which was successful). Now, git push still fails with
I remember that you suggested to merge instead of push --force. After merging the branch to the local develop and pushed the develop to the remote develop, I tried to push the local neutron-inelastic to the remote neutron-inelastic, which still fails (i.e., error above). Probably I missed something here. Thank you for your help. |
@whokion Please don't do any rebasing once reviews have started. You should instead As a reminder for why merging is preferred once a PR has left draft mode: rebasing destroys the CI logs and messes with the review history. |
Thanks @sethrj! Good to know and to learn. I just to push a new |
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.
Looks great, @whokion ! Sorry for taking so long to review. If you think the consolidation of the XS calculators should wait, I accept that, but it looks like a pretty easy change to make now.
/*! | ||
* Calculate neutron inelastic cross sections from NeutronInelasticData | ||
*/ | ||
class NeutronInelasticMicroXsCalculator |
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.
@whokion The data and implementation seem identical to NeutronElasticData (setting aside capture for now). Would it be OK to define:
template<Ownership W, MemSpace M>
struct NeutronXsData
{
//! Microscopic (element) cross section data (G4PARTICLEXS/neutron/elZ)
ElementItems<GenericGridData> micro_xs;
// Backend data
Items<real_type> reals;
// (plus copy constructor, bool)
};
and then replace the micro_xs
and (if needed) reals
in both NeutronElasticData
and NeutronInelasticData
with an instance of that class? E.g.
template<Ownership W, MemSpace M>
struct NeutronElasticData
{
// ...
NeutronXsData<W, M> xs;
// ...
};
Then NeutronElasticMicroXsCalculator
could be renamed NeutronMicroXsCalculator
and used for both elastic and inelastic. Is there anything I'm overlooking that would make this complicated?
@sethrj Thanks for reviewing and extra comments. Regarding to the consolidation of the Xs calculators, your suggestion looks good. However, I still want to postpone it to later as there may be an extension for the inelastic interaction cross sections as there are several special elements which have isotope-dependent cross sections. Once the capture cross sections (which are mostly isotope-dependent and are different structure from elastic ones), I would reconsider which way is better to consolidate; elastic vs. inelastic (element-dependent) or inelastic vs. capture (isotope-dependent). Anyway, I pushed a branch name, neutron-inelastic-temp2 (and sorry again for the extra rebase/merging treatment). |
@sethrj did you incorporate the changes from the neutron-inelastic-temp2 branch? |
God damn it, I pushed to my origin instead of Soon's. Thanks for the catch @amandalund . |
Description
This MR includes