Skip to content
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

Adding MGimp processor and maximum number of split per particle #110

Merged
merged 6 commits into from
Jan 25, 2024

Conversation

valeriaRaffuzzi
Copy link
Member

@valeriaRaffuzzi valeriaRaffuzzi commented Jan 24, 2024

I had done this for the PHYSOR paper few months ago, and I think it could be useful to someone.

I know that the collision operator as a whole will need refactoring in the future, but for the moment someone might want to do implicit MG MC, so I added neutronMGimp collision processor. For now, this only does weightwindows, but all the other standard techniques can be easily added.

This PR also adds a property to the particle, i.e., the number of splits it had. This property is zeroed when a particle generates new ones, so there is no limit on the splits of the particle family. However this is a small change and it could easily be added. Actually, I'm starting to think it would be a better idea!

In general, this is useful when using extreme variance reduction, to reduce the computational time of the calculations.

For now this number is fixed to 1000, but it can be easily made to be read from the input if you think it's better (it probably is honestly).

Copy link
Collaborator

@Mikolaj-A-Kowalski Mikolaj-A-Kowalski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I would just wait with merging until the spawnParticle is merged so we can apply changes it brings here as well. I don't know if you agree?

As for inheriting the split, I fear that limiting the splits per family may be quite difficult (since 2ndary particles have no way to communicate to find out how many splits took place after they were generated, they only know about number of splits until they were born). For that reason I am not sure that inheriting the split count would bring much? (I am guessing really) So I thing we can leave things as they are.

The last minor point is that we should probably also update the manual to include the new processor.


! This value must be at least 2
mult = ceiling(p % w/maxWgt)

! Limit maximum split
if (mult > maxSplit - p % splitCount + 1) then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (mult > maxSplit - p % splitCount + 1) then
if (mult + p % splitCount > maxSplit) then

Maybe this condition will be a bit more readable?
I had a bit of trouble to work out the intention behind this if statement. It does limit number of 2ndary particles created by splitting so they do not exceed maxSplit, right? Or did I misunderstood something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure! That was the intention, I might have made it more convoluted than it needed to be! Thanks

Comment on lines 34 to 39
! Nuclear Data
!use nuclearData_inter, only : nuclearData
!use perMaterialNuclearDataMG_inter, only : perMaterialNuclearDataMG

! Cross-section packages to interface with nuclear data
!use xsMacroSet_class, only : xsMacroSet, xsMacroSet_ptr
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! Nuclear Data
!use nuclearData_inter, only : nuclearData
!use perMaterialNuclearDataMG_inter, only : perMaterialNuclearDataMG
! Cross-section packages to interface with nuclear data
!use xsMacroSet_class, only : xsMacroSet, xsMacroSet_ptr

I think these pointless commented out lines have been in the file for too long ;-)

Comment on lines 44 to 58
!!
!! Standard (default) scalar collision processor for MG neutrons
!! -> Preforms implicit fission site generation
!! -> Preforms analog capture
!! -> Treats fission as capture (only implicit generation of 2nd-ary neutrons)
!! -> Does not create secondary non-neutron projectiles
!!
!! Settings:
!! NONE
!!
!! Sample dictionary input:
!! collProcName {
!! type neutronMGimp;
!! }
!!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we wish to mention the support for weight windows more explicitly here?
Also I think there is an extra weightWIndows keyword in the dictionary which we probably should include in the Sample dictionary input

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right, I tend to forget the docs :/

I will add this, and to the manual too.

Comment on lines 351 to 353
if (mult > maxSplit - p % splitCount + 1) then
mult = maxSplit - p % splitCount + 1
end if
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as in CE case: Perhaps the meaning of the condition in the if may be clearer if restructured?

@@ -55,6 +55,7 @@ module particle_class
integer(shortInt) :: matIdx = -1 ! Material index where particle is
integer(shortInt) :: cellIdx = -1 ! Cell idx at the lowest coord level
integer(shortInt) :: uniqueID = -1 ! Unique id at the lowest coord level
integer(shortInt) :: splitCount = 0 ! Counter of number of splits
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be a silly question, but doe we use the splitCount in the particleState?
Sorry, I just get paranoid lately that more and more data gets into particleState and it just grows on...
I guess it will become necessary if the particles inherit the splitCount from their primogenitor, but otherwise maybe we need spliCount only in the particle?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's fair. I think I added it just in case we wanted to pursue the 'family' weight thing, in which case it could have been useful, but it can surely be avoided for the moment.

@valeriaRaffuzzi
Copy link
Member Author

Looks good to me! I would just wait with merging until the spawnParticle is merged so we can apply changes it brings here as well. I don't know if you agree?

Definitely, especially because the spawnParticle as expected generated a few conflicts! It will be a quick fix now at least.

@valeriaRaffuzzi
Copy link
Member Author

Now this is all compatible with the recent changes.

NOTE: I modified the collisionProcessors and made maxSplit their property rather than a universalVariable, such that it can be modified.

Copy link
Collaborator

@Mikolaj-A-Kowalski Mikolaj-A-Kowalski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks!

@valeriaRaffuzzi valeriaRaffuzzi merged commit 16ee2a7 into CambridgeNuclear:main Jan 25, 2024
5 checks passed
@valeriaRaffuzzi valeriaRaffuzzi deleted the varRed branch January 25, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants