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

FDTD Solver / Free Electron Laser for ALPINE #289

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

manuel5975p
Copy link
Contributor

Open questions

  • How to incorporate boundary conditions? They are incompatible with ippls BCType concept
  • Should Electrostatic and Electromagnetic Solvers implement the same interface?

Design

@manuel5975p manuel5975p force-pushed the migrate_from_working_experiments branch from db78f30 to 5d6c974 Compare May 18, 2024 12:56
@manuel5975p
Copy link
Contributor Author

Update: Now everything is in the /fel directory
I also added a matrix type in Types/Matrix. The lorentz transform classes depend on it.

@aaadelmann
Copy link
Member

There are still a few open comments from Sonali to be addressed.

@s-mayani s-mayani self-requested a review June 24, 2024 14:24
/* Type of the bunch which is one of the manual, ellipsoid, cylinder, cube, and 3D-crystal. If
* it is manual the charge at points of the position vector will be produced.
*/
// std::string bunchType_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

See previous comment: "why is this commented? not needed?"

// std::vector<FieldVector<scalar> > position_;
FieldVector<scalar> position_;

/* Number of macroparticles in each direction for 3Dcrystal type. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

see previous comment: this 3D crystal type thing is inherited from mithra I assume? is it actually supported here? if so I would add some explanation of what it means

Comment on lines +553 to +555
// printmessage(std::string(__FILE__), __LINE__, std::string("Warning: The number of
// particles in the bunch is not a multiple of four. ") +
// std::string("It is corrected to ") + std::to_string(bunchInit.numberOfParticles_) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this commented? if not necessary, please remove

Comment on lines +578 to +582
/* Check the bunching factor. */
if (bunchInit.bF_ > 2.0 || bunchInit.bF_ < 0.0) {
// printmessage(std::string(__FILE__), __LINE__, std::string("The bunching factor can not be
// larger than one or a negative value !!!") ); exit(1);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this if is not doing anything

Comment on lines +604 to +607
// else
// return ( randomNumbers[ n * 2 * Np/ng + m ] );
// TODO: Return halton properly
// return ( halton(n,m) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

why commented? if not needed, remove

if (std::isnan(iterQ->rnp[2])) {
std::cerr << iterQ->gb[2] << ", " << g << ", " << iterQ->rnp[2] << ", " << bunch_.zu_
<< ", " << frame_beta << "\n";
std::cerr << __FILE__ << ": " << __LINE__ << " OOOOOF\n";
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 message should be more informative

for (auto iterQ = chargeVectorn_.begin(); iterQ != chargeVectorn_.end(); iterQ++) {
Double g = std::sqrt(1.0 + iterQ->gb.squaredNorm());
if (std::isinf(g)) {
std::cerr << __FILE__ << ": " << __LINE__ << " inf gb: " << iterQ->gb << ", g = " << g
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 message should be more informative, unless it's just there for debug purposes

fel/FreeElectronLaser.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this Matrix type used? Is this a replacement of the ippl::Vector<ippl::Vector<T,3>,3> type normally used as Matrix_t in the solvers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Used in the struct LorenzFrame, which is not used anywhere in the code in fel.
Do we want this struct and this type in ippl?

Copy link
Collaborator

@s-mayani s-mayani Jun 24, 2024

Choose a reason for hiding this comment

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

still needs to be divided into .h and .hpp, if we have decided to still go with that structure

Copy link
Collaborator

Choose a reason for hiding this comment

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

Devided in to FDTDSolverBase, StandardFDTDSolver and NonStandardFDTDSolver, all with .h and .hpp file

Comment on lines 48 to 50
// return pear<ippl::Vector<int, 3>, ippl::Vector<T, 3>>{ippl::Vector<int, 3>{5,5,5},
// ippl::Vector<T, 3>{0,0,0}}; printf("%.10e, %.10e, %.10e\n", (inverse_spacing *
// spacing)[0], (inverse_spacing * spacing)[1], (inverse_spacing * spacing)[2]);
Copy link
Collaborator

@s-mayani s-mayani Jun 25, 2024

Choose a reason for hiding this comment

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

commented stuff is just for debug purposes or necessary to keep?

for (unsigned k = 0; k < 3; k++) {
fracpos[k] = gridpos[k] - (int)ipos[k];
}
// TODO: NGHOST!!!!!!!
Copy link
Collaborator

Choose a reason for hiding this comment

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

please make this TODO more informative

Comment on lines 163 to 174
// printf("Scatterdest: %.4e, %.4e, %.4e\n", from_grid.second[0], from_grid.second[1],
// from_grid.second[2]);
for (int d = 0; d < 3; d++) {
// if(abs(from_grid.first[d] - to_grid.first[d]) > 1){
// std::cout <<abs(from_grid.first[d] - to_grid.first[d]) << " violation " <<
// from_grid.first << " " << to_grid.first << std::endl;
// }
// assert(abs(from_grid.first[d] - to_grid.first[d]) <= 1);
}
// const uint32_t nghost = g.nghost();
// from_ipos += ippl::Vector<int, 3>(nghost);
// to_ipos += ippl::Vector<int, 3>(nghost);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this doing? remove if not necessary anymore

Copy link
Collaborator

Choose a reason for hiding this comment

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

add more comments in general

Comment on lines +60 to +63
FourField A_n;
FourField A_np1;
FourField A_nm1;
scalar dt;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Previous review comment still applies: Are these attributes supposed to be public?
It would also make the code more readable to have them at the end of the code block (like in the first public block you have the attributes at the end before the private).

Copy link
Collaborator

Choose a reason for hiding this comment

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

These are protected variables in FDTDSolverBase now.

* @tparam FourField
*/
template <typename EMField, typename FourField, fdtd_bc boundary_conditions = periodic>
class NonStandardFDTDSolver : Maxwell<EMField, FourField> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add more comments to this class / documentation

@@ -186,14 +187,15 @@ namespace ippl {
"ParticleAttrib::gather", policy_type(0, *(this->localNum_mp)),
KOKKOS_CLASS_LAMBDA(const size_t idx) {
// find nearest grid point
vector_type l = (pp(idx) - origin) * invdx + 0.5;
vector_type l = (pp(idx) - origin) * invdx + (1.0 - offset);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this solution okay with everyone? @srikrrish @matt-frey

Copy link
Member

@srikrrish srikrrish Dec 3, 2024

Choose a reason for hiding this comment

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

It seems to me the offset is always used with the default value of 0.5 and hence boils down to the old implementation. Because the non-class function gather didn't pass the argument offset and I don't see anywhere else in the changed files the gather function from ParticleAttrib class being directly called. So it seems to me that it was once introduced but forgot to remove? Or did I overlook somewhere?

return ret;
}
template <typename view_type, typename scalar>
KOKKOS_FUNCTION void scatterToGrid(const ippl::NDIndex<3>& ldom, view_type& view,
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment from previous review still applies: the scatter is repeated here and also in the FreeElectronLaser.cpp, would be nice to avoid this by putting it in some common file maybe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants