-
Notifications
You must be signed in to change notification settings - Fork 22
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
TMS #121
TMS #121
Conversation
One initial comment: do we want having a temperature in a material definition to activate TMS? I think there is something to be said for having a separate TMS flag in addition to temperature, as we essentially have for S(a,b). When we come to doing multiphysics etc, temperature seems like a natural variable to have around even if there are places we don't want to apply TMS. |
CollisionOperator/CollisionProcessors/collisionProcessor_inter.f90
Outdated
Show resolved
Hide resolved
NuclearData/ceNeutronData/aceDatabase/aceNeutronDatabase_class.f90
Outdated
Show resolved
Hide resolved
NuclearData/ceNeutronData/aceDatabase/aceNeutronDatabase_class.f90
Outdated
Show resolved
Hide resolved
NuclearData/ceNeutronData/aceDatabase/aceNeutronDatabase_class.f90
Outdated
Show resolved
Hide resolved
NuclearData/ceNeutronData/aceDatabase/aceNeutronDatabase_class.f90
Outdated
Show resolved
Hide resolved
NuclearData/ceNeutronData/aceDatabase/aceNeutronDatabase_class.f90
Outdated
Show resolved
Hide resolved
NuclearData/ceNeutronData/aceDatabase/aceNeutronNuclide_class.f90
Outdated
Show resolved
Hide resolved
!! Errors: | ||
!! FatalError if material kT is smaller than the nuclide kT | ||
!! | ||
subroutine updateTotalTempNucXS(self, E, kT, nucIdx) |
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 this be given a more descriptive name? I suppose for me Temp has an annoying double connotation. Reading the comments, it isn't clear how the name relates to the function other than TMS - maybe updateTotalTMSNucXS?
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 think calling it temperature majorant is quite standard terminology. Also in aceNeutronNuclide we have a function to calculate a temperature majorant used by both TMS and DBRC. I would say it is clear enough.
! Obtain xss | ||
call mat % getMacroXSs(xss, p) | ||
|
||
flux = p % w / xss % total |
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 happened that make tallies need to do the extra call to mat? Or rather, why does xsData % getTotalMatXS no longer work? Is there any way to preserve it?
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 really, because in TMS getTotalMatXS gets you the material temperature majorant, while getMacroXSs retrieves the material macroscopic xss at the relative energy. I should probably comment the code better to emphasize 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 think this point should be elaborated in the doc comment of the getTotalMatXS
and getMacroXSs
. It is a bit difficult to understand what cross-sections are obtained bu which call... however I am not sure how to make it cleaner at this moment.
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.
Perhaps there is no way around introducing some kind of getTrackingMatXs
? This double meaning between the getMacroXS
and getTotalMatXS
is going to cause confusion and bug down the line I fear.
All of this obviously gets a bit confusing since the cross-sections have now become random variables. However, this confusion is not specific to TMS since it appears in URR as well. Basically we need to somehow separate the 'sampled' XSs at a particular state of the flight from a deterministic supremum on their values (TrackingXS
?)
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.
Yeah I agree that it should be made more clear. I tried to keep the code as simple as possible but clearly it's not an easy task and I might have overcomplicated interpretation.
The fact that colling getTotalMatXS
is not safe is not great. I'll have a look.
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 don't think separating deterministic vs random xss is a good way to do it. When using URR there is no point in asking for the deterministic xss. Same with TMS, one either wants the temperature majorant or the relative energy xss. What I am thinking for now is:
updateTrackingMatXS:
if TMS -> get Temperature majorant
else -> get Total
updateTotal:
if TMS -> get relative energy xss (when doing the relative energy sampling one might as well get the whole package) and return total
else -> get Total as always
updateMacroXSS:
if TMS -> get relative energy xss
else -> get Macro as always
So as far as TMS is concerned, updateTrackingMatXS gets the deterministic majorant and updateTotal and updateMacro give the relative energy xss. When TMS is not used, there is no difference between updateTracking and updateTotal.
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 for this. My most major comment is just on keeping temperature as a separate variable which I think we would like to distinguish without demanding TMS (e.g., for multigroup calculations where we might eventually have multiphysics).
Thanks for reviewing! I am ok with changing keyword to, say, 'tms' instead of 'temp'. But I don't think we can easily skip the materialMenu step, not I think it is really an issue to go through it! |
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.
Submitting the review but I didn't manage to finish before the changes.
Will go over it again.
CollisionOperator/CollisionProcessors/collisionProcessor_inter.f90
Outdated
Show resolved
Hide resolved
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 think the biggest problem to address is to clarify the nuclear data interface in the presence of random cross-sections.
Basically we need to say clearly which functions return maximum value of the cross-section and which return a value obtained by a sample. I have a suspicion that we can call the maxima 'tracking' cross-section and in all other contexts functions would return sampled values? But I might be missing some problems.
I also believe having a paragraph to describe how we handle the random cross-sections (in the docs) would be really helpful, because I have a feeling it is difficult to make sense of what is going on from the code alone.
@@ -225,13 +223,32 @@ subroutine reportInColl(self, p, xsData, mem, virtual) | |||
! Return if invalid bin index | |||
if (binIdx == 0) return | |||
|
|||
! Return if collision is virtual but virtual collision handling is off |
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.
! Return if collision is virtual but virtual collision handling is off |
The comment seems to be dead. This handling was moved up the function?
! Obtain xss | ||
call mat % getMacroXSs(xss, p) | ||
|
||
flux = p % w / xss % total |
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 think this point should be elaborated in the doc comment of the getTotalMatXS
and getMacroXSs
. It is a bit difficult to understand what cross-sections are obtained bu which call... however I am not sure how to make it cleaner at this moment.
! Obtain xss | ||
call mat % getMacroXSs(xss, p) | ||
|
||
flux = p % w / xss % total |
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.
Perhaps there is no way around introducing some kind of getTrackingMatXs
? This double meaning between the getMacroXS
and getTotalMatXS
is going to cause confusion and bug down the line I fear.
All of this obviously gets a bit confusing since the cross-sections have now become random variables. However, this confusion is not specific to TMS since it appears in URR as well. Basically we need to somehow separate the 'sampled' XSs at a particular state of the flight from a deterministic supremum on their values (TrackingXS
?)
@Mikolaj-A-Kowalski @ChasingNeutrons I can see you didn't read the PR description!!! Does anyone have an intuition on this? |
Sorry. It does appear that I have missed this point in the description. The functions should use the LAB frame incident energy, not the relative energy. Basically the sampled energy is 'kind off' an energy in the CoM frame of the collision. If it was used weird things might appear. |
Apologies for adding yet more stuff! I implemented all of the reviews:
Let me know if this works for you! |
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 guess the only thing left to do is integrate and perhaps open an issue about trying to rework cache so it is clearer when we have time/ideas.
Here is the basic implementation of TMS. The main modifications are:
NOTE: TMS is applied to the materials where 'temp' is specified in the input file, and outside S(a,b) and ures energy range with a margin such that the relative energy doesn't fall into those ranges
NOTE2: in aceDB % init, there's a check with FatalError if the material temperature specified is smaller or equal a nuclide temperature
Other modifications:
NOTE3: This implementation might still be suboptimal in terms of speed. For example, trying to use TMS on water (silly anyway), when deltaT > 50K the code slows down a lot because of the massive thermal temprature majorant and lots of rejections. Hopefully this can be fixed in the future.