-
Notifications
You must be signed in to change notification settings - Fork 218
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
SAXS plugin implementation using electron density #3134
SAXS plugin implementation using electron density #3134
Conversation
#2966 passed the compile test and is also using the API so there should be a way to make it work here as well but for now, I don't know how to do it. |
I talked with @psychocoderHPC today and we decided that we want to merge it without a 100% correct coordinate transform. I have hidden the functionality that depended on it by moving the settings from the I guess this means, it is not a WIP anymore. Can't wait for your reviews 😄 |
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.
only a few small changes are required but you can wait if someone else is annotating something before you apply the changes.
@PrometheusPi @HighIander please review this PR. |
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.
Great work @pordyna ! I only looked through part of the code so far, here are some notes.
b94a6e3
to
ebf9fa9
Compare
@pordyna Your travis-CI tests fail in the C++ part due to an too old CMake version used. Error message: $ cmake $TRAVIS_BUILD_DIR/include/pmacc -DALPAKA_ACC_CPU_B_OMP2_T_SEQ_ENABLE=ON
CMake Error at CMakeLists.txt:27 (cmake_minimum_required):
CMake 3.11.4 or higher is required. You are running version 3.11.0
-- Configuring incomplete, errors occurred!
The command "cmake $TRAVIS_BUILD_DIR/include/pmacc -DALPAKA_ACC_CPU_B_OMP2_T_SEQ_ENABLE=ON" exited with 1.
$ make -j 2
make: *** No targets specified and no makefile found. Stop.
The command "make -j 2" exited with 2.
store build cache Just to be sure, I restarted both failed jobs - but I am pretty sure that that the mismatching version will cause a failure again. |
@PrometheusPi @pordyna I believe the issue with failing builds is due to cmake version requirements update in #3139 . Rebasing the branch on top of the current |
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.
This is a not yet complete review.
I did not go through
Saxs.hpp
Saxs.kernel
SaxsWriter.hpp
In general, the main code looks very good. I made some comments but nothing severe.
The new example file looks like a copy of the LWFA example.
This creates a lot of code and test duplications.
Could you add a SAXS example to an existing example - or did you adjust the param files so significantly, that adding a new exmaple is well motivated. Even if the later is well motivated, there are many param files added that do not differ from the default. Please remove those.
Ideally, you introduce a distinct new SAXS example (suited for testing the SAXS plugin against known analytical solutions) or you add a SAXS compile test to an existing (LWFA) example.
share/picongpu/examples/Saxs/include/picongpu/param/species.param
Outdated
Show resolved
Hide resolved
share/picongpu/examples/Saxs/include/picongpu/param/speciesDefinition.param
Outdated
Show resolved
Hide resolved
share/picongpu/examples/Saxs/include/picongpu/param/speciesInitialization.param
Outdated
Show resolved
Hide resolved
share/picongpu/examples/Saxs/include/picongpu/param/starter.param
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.
Furthermore, I have the impression that you at two points mix PIC units with SI units, which will result in a (globally) incorrectly scaled result. Relatively, everything should be correct, but the total quantity should be off by a fixed factor. (This is not bad for meter to angstrom, bit for the classical electron radius, since is is not just a 10^10).
@sbastrakov and @psychocoderHPC I checked everything except the SaxsWriter.hpp
- this is more in your domain of expertise.
I will perform some run time tests, as soon as the example is clean up.
ebf9fa9
to
619efaa
Compare
Alright, going forward 😄 ! I removed the Saxs example for now. I think that a proper example involving some interesting physics could come with an another pull request. For example, one with an artificial lattice, as suggested by @sbastrakov, or maybe a proper simulation with a prestructured target. There is a I tried to address all of your comments but please let me know if I forgot something. Fix for the travis style test coming in a moment 😉 |
Great @pordyna ! Thx @PrometheusPi for a detailed review, I would also like to have another look, will put my comments a little later. |
@pordyna looks like you might have forgotten to rebase your branch on top of the current |
@sbastrakov I did rebase it and I just looked into the |
@PrometheusPi @psychocoderHPC I have some jupyter notebooks which I used to compare plugin output with python FFT. What is the best way to upload them here? |
In case these are for the LWFA example with your new |
Yes exactly, one for |
It looks like the Access_Type -> Access change in openpmd will come with the next release. |
I tried building docs localy but I get
I'm sure I'm just doing something wrong, it used to work before. Full log: |
95d10d3
to
3c9727b
Compare
@HighIander Could you please comment on @pordyna question regarding the double slit? |
@pordyna We just discussed about your pull request in the dev meeting. We will merge and will investigate the discrepancy between theory and simulation later. |
@psychocoderHPC Regarding the ci, what should we do? |
|
0f87e13
to
bc0803d
Compare
@psychocoderHPC , @PrometheusPi Are we ready to merge, or is there anything left to do ? :) |
I will have a look to this PR tomorrow. |
we will wait until monday for #3316. This will allow that the ci is testing the plugin. The old ci has no openpmd support therefore this PR is not checked. |
@pordyna I am fine with merging (sorry was on vacation) - please rebase your code. |
bc0803d
to
88d9c0e
Compare
I see that all 60/60 CI jobs failed 😅 @psychocoderHPC @sbastrakov could you have a look? 😃 |
@pordyna I think there may be a confusion between |
88d9c0e
to
e69b199
Compare
e69b199
to
b177f35
Compare
Suggested changes from PR reviewers (runtime error on write) -> pmacc::math, GridController access moved rollback change causing runtime error fix axis swap 120x120 -> 128x128 split -> distribute fix wrong density profile adapt thresholds for the 3D case fix wrong dtype in the output after multiplication pylint remove 3.cfg Update docs/source/usage/plugins/xrayScattering.rst Co-authored-by: Richard Pausch <[email protected]> Update docs/source/usage/plugins/xrayScattering.rst Co-authored-by: Richard Pausch <[email protected]> don't use \x0bScalar more info on output in docs fixes for sphinx doc openPMD --> ::openPMD math::Pi -> pmacc:math::Pi
b177f35
to
dfe8a99
Compare
@psychocoderHPC The CI tests passed. I think we are ready to merge. 🚀 |
WIP (status 08.05.2020):
This plugin calculates scattering amplitude from a SAXS experiment (Small Angle X-ray Scattering). It can compute the complex amplitude for all charged particles, it is meant to be used with PIC electrons and ions.
It is a continuation of the project started by @ejcjason in #2952 and maintained by @sbastrakov in #2973. The scattering is now calculated from particle density ( bound electron density for ions) that is provided by the
particleToGrid
algorithm. This simplifies the parallelization compared to the direct calculation from the particle data. Also now the particle shape is taken into account since it's used to derive the averaged field.What else changed since #2973:
openPMD
standard. It's written by theSaxsWriter
class, using the openPMD API.SaxsWriter
is based on @franzpoeschel's CUDA 9.0+, GCC 5.1+, Boost 1.65.1+ #2961.saxs.param
file now.The plugin is meant to be used with 3D simulation since otherwise it only calculates a scattering on a two-dimensional sheet. Nevertheless, it compiles and runs in 2D as well.
As mentioned, the beam alignment is not working correctly. It is based on a coordinate transformation from the PIC coordinate system into the comoving system of the probing beam. For now, I could verify that rotation is working properly. Unfortunately, translation is not yet correct. Since a correct cell position in the beam system is needed to obtain the beam profile (shape) at a given cell, for now only a plane wave probe will give correct physical results.
Things that should be still done:
Things that still could be done:
densityProfiles::FreeFormulaImpl
.I had also an idea of generalizing the
SaxsWritter
into an abstract class for output writing that could be specialized either by inheritance or templates to use in different scenarios, plugins. Could be also used in this plugin for the eventual beam intensity output.Example output:
At the time I'm only once a week at HZDR and I will not be able to work on the plugin for some time, I'm starting these pull request now, although some features are not giving 100% correct results, to summarize the current status.
@ComputationalRadiationPhysics/picongpu-developers