add method to radiation python module to load distributed amplitudes #4752
Conversation
PrometheusPi
added
component: plugin
|
Code has been tested. |
| for i, direction in enumerate(["x", "y", "z"]): | ||
| tmp = h_distAmp[direction][:, :, :] | ||
| self.rad_series.flush() | ||
| distAmp[:, :, :, i] = tmp |
There was a problem hiding this comment.
distAmp[:, :, :, i] is a non-contiguous memory slice, right? If not, you could load directly into that buffer, but otherwise the current implementation with an intermediate load buffer is fine.
There was a problem hiding this comment.
Good point. As a physicist I would prefer keeping the polarization as my last component - but this is simply due to common convention. I agree with you that directly writing into a chunk of continuous memory would be more elegant and also easier to read.
I will change the memory layout according to your suggestion.
There was a problem hiding this comment.
I think that it's okay to have a different memory layout than on the disk. I just wanted to confirm that I interpreted this correctly; probably a short comment is all that's needed here.
There was a problem hiding this comment.
Okay - then I will just a comment. Thanks for the feeback @franzpoeschel.
There was a problem hiding this comment.
You could, of course, use fancy numpy methods to do so, e.g., create your array such that you can load contiguously and afterwards do a np.moveaxis() or .transpose() or the like.
PrometheusPi
force-pushed
the
add_distrRad_toPythonModule
branch
from
4f702f7
to
4d15fc8
Compare
steindev
added
the
changelog
|
ping @PrometheusPi |
|
@PrometheusPi please rebase against dev branch because pre commits has changed and check now python code too |
|
Steering things up... |
|
@PrometheusPi is this PR still of interest? |
|
Yes, I will update it now |
PrometheusPi
force-pushed
the
add_distrRad_toPythonModule
branch
from
4d15fc8
to
07cdac3
Compare
|
I will not be able to work on this PR until #5105 is merged. |
PrometheusPi
force-pushed
the
add_distrRad_toPythonModule
branch
2 times, most recently
from
e26dcc1
to
7dc8d12
Compare
|
@steindev @psychocoderHPC @ikbuibui ready for review |
| # get shape of distributed amplitude | ||
| shapeDistAmp = h_distAmp["x"].shape | ||
| # add 3 vector components | ||
| shapeDistAmp.append(3) |
There was a problem hiding this comment.
What's the magic number 3? Maybe a named constant would be nice.
There was a problem hiding this comment.
3 is for the 3 spacial dimensions
Since there is no const uint in python, I personally favor using literals instead.
What would you prefer here to make clear that is a never changing value?
There was a problem hiding this comment.
@chillenzer I used a class variable to make the global scope of that 3 clear. Is this in agreement with what you wanted?
|
I am fine with the changes. Would you please squash (and rebase, if necessary)? I guess @chillenzer's comment is resolved as well. Then I will merge. |
PrometheusPi
force-pushed
the
add_distrRad_toPythonModule
branch
from
967ef18
to
d942c0f
Compare
|
@steindev squashed to 2 commites |
This pull request adds a new method to the radiation plugin python module to load all distributed radiation amplitudes if available.