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

1D tally maps #122

Merged
merged 5 commits into from
May 16, 2024
Merged

Conversation

valeriaRaffuzzi
Copy link
Member

Just tidying the maps a bit: if we want to keep the separation between tallyMaps and tallyMaps1D, which is useful for the multiMaps, I think some order makes things clearer.

The main changes are:

  • moving all the 1D maps and tests into a new folder: TallyMaps/Maps1D
  • renaming sphericalMap -> sphericalRadMap, given that only the radial dimension was mapped. If one wants to make a multidimensional sphericalMap, then that would be a TallyMap
  • Making a version of cylindricalMap with only the radial components: /Maps1D/cylindricalRadMap
  • Adding a directionMap to map the direction of flight of the particles. For now only a linear grid (0-2pi) is allowed, and the user needs to choose the 2D plane of interest.

I forgot to update the User manual, I'll do it after the reviews to make sure I am not missing anything given that a lot has to be changed.

Comment on lines 167 to 173
do i = 1, self % N
! Print lower bin boundary
call out % addValue(self % bounds % bin(i) + PI)

! Print upper bin boundar
call out % addValue(self % bounds % bin(i + 1) + PI)
end do
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
do i = 1, self % N
! Print lower bin boundary
call out % addValue(self % bounds % bin(i) + PI)
! Print upper bin boundar
call out % addValue(self % bounds % bin(i + 1) + PI)
end do
do i = 1, self % N
! Print lower bin boundary
call out % addValue(self % bounds % bin(i))
! Print upper bin boundar
call out % addValue(self % bounds % bin(i + 1))
end do

NOTE: otherwise the bins don't coincide with the bins of the results.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lines 379 - 382: also remove PI from the printed bin values

@ChasingNeutrons
Copy link
Collaborator

One question before I dive in: would it be neater to have a generic radial map where you can specify which dimensions/plane/axis to account for? For example, it could be xy, xz, yz, or xyz (spherical). This is as opposed to having separate cylindrical and spherical radial maps.

@valeriaRaffuzzi
Copy link
Member Author

One question before I dive in: would it be neater to have a generic radial map where you can specify which dimensions/plane/axis to account for? For example, it could be xy, xz, yz, or xyz (spherical). This is as opposed to having separate cylindrical and spherical radial maps.

Yes that should be easy enough! Let me give it a quick try.

@@ -0,0 +1,153 @@
module directionMap_test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps more descriptively it should be azimuthalDirectionMap or something similar? This would be to distinguish it from an eventual polar direction map.

@@ -0,0 +1,194 @@
module directionMap_class
Copy link
Collaborator

Choose a reason for hiding this comment

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

Meant to put that previous comment here...

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 worries, I understood! It's a bit of a long name. I personally would leave it as it is until a polarMap comes by. Also because you can map direction on any plane with this, so it's quite general.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Going from -pi to pi it is still the azimuthal component of direction, which is not inherent to any plane. I would suggest it's better to rename it in advance of having polar because it is more descriptive and helps us not forget. Even something like azimuthDirMap.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, it might be even more convenient to leave it generic and add the possibility to modify the angle range to anything within -PI to PI. So one could tally -PI to PI on a plane and 0 to PI on another

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose it would still be a fraction of the azimuthal plane. Wouldn't the polar equivalent need some weighting by a sine? But, in principle, the range could be limited. I imagine that might make things a bit messier and also wouldn't be requested very frequently so I wouldn't worry much about it.

One more thing: which direction would the 0 angle correspond to in a given plane? Or pi/2? It would be good to mention these explicitly at least in the comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that if you want to integrate over a polar angle you need to add the sign weighting as a Response, rather than in the tally Map. Now I added a custom min and max angle, so that it generalises better.

self % DIM2 = Y_AXIS

case default
call fatalError(Here, 'Keyword orientation must be x, y or z. It is: '//type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This or the cases would need to change.

spherical = .true.

case default
call fatalError(Here, 'Keyword orientation must be x, y, z or nothing for spherical. It is: '//type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This error is inconsistent with the spherical case above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add it might be good idea to include xyz keyword in the doc comment. As suggested above

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 with just couple of minor issues.
Thanks for the addition!

Comment on lines 15 to 17
!!
!! Maps the particle direction in linear bins in the range 0-360 degrees
!!
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this description is not up to date. The maps works in -pi pi range in radians, right?
We should also mention with respect to what the angle is measured and highlight that user can specify the plane?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it was. Now I changed it to be defined in degrees from -180 to 180, given that degrees are more intuitive I think. I added those comments.

spherical = .true.

case default
call fatalError(Here, 'Keyword orientation must be x, y, z or nothing for spherical. It is: '//type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add it might be good idea to include xyz keyword in the doc comment. As suggested above

!! Sample Dictionary Input:
!! radialMap {
!! type radialMap;
!! axis x; // Optional. Default is spherical map
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
!! axis x; // Optional. Default is spherical map
!! axis x; // Optional. Default is 'xyz' (spherical map)

Comment on lines 98 to 103
! Check & load origin
if (spherical) then
call dict % getOrDefault(self % origin, 'origin', [ZERO, ZERO, ZERO])
else
call dict % getOrDefault(self % origin, 'origin', [ZERO, ZERO])
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.

We should check the size of the origin? I am not sure if it is done.
Also perhaps we should always use all 3 coordinates? Origin is a point in space, one coordinate will be redundant, sure, but I know I will be getting confused about what are the axis if we switch two two only for cylinders ;-) [Basically like it is done for cylindrical surfaces]

Copy link
Member Author

@valeriaRaffuzzi valeriaRaffuzzi May 5, 2024

Choose a reason for hiding this comment

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

I guess I was imitating what done in cylinderMap, where the origin only had two entries. I am changing both cylinderMap and radialMap to read origins with 3 entries to avoid confusion for the user.

Comment on lines 216 to 217
! Calculate the distance from the origin
r = norm2(state % r(self % axis) - self % origin)
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
! Calculate the distance from the origin
r = norm2(state % r(self % axis) - self % origin)
! Calculate the distance from the origin
r = norm2(state % r(self % axis) - self % origin(self % axis))

Assuming that we are using size 3 origin.

@valeriaRaffuzzi
Copy link
Member Author

I'll ho ahead and merge this, given that it was approved and only a couple of aesthetic things changed since!

@valeriaRaffuzzi valeriaRaffuzzi merged commit a7b7218 into CambridgeNuclear:main May 16, 2024
5 checks passed
@valeriaRaffuzzi valeriaRaffuzzi deleted the angleMap branch May 16, 2024 15:37
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.

None yet

3 participants