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

Add Splines of order 4 to 7 and support for all heFFTe MPI communication backends #799

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

Conversation

rodrigoacb
Copy link

This commits add BSplines of order 4 to 7 following the polynomial definitions on deserno1998*

This also extends the available backends available in heFFTe using a enum class to pick the chosen reshape/communication pattern. 

This changes were done in cooperation with @rhalver

  • Markus Deserno, Christian Holm; How to mesh up Ewald sums. I. A theoretical and numerical comparison of various particle mesh routines. J. Chem. Phys. 8 November 1998; 109 (18): 7678–7693. https://doi.org/10.1063/1.477414

Rodrigo Bartolomeu and others added 2 commits October 30, 2024 16:56
@streeve
Copy link
Member

streeve commented Oct 31, 2024

Thanks for the improvements! Do you mind splitting into two PRs? I think I'll want someone else to at least co-review the spline details, but the FFT should be quick to merge

@@ -97,13 +95,21 @@ struct is_matching_array<
: public std::true_type
{
};
enum class FFTCommPattern : unsigned int
Copy link
Member

Choose a reason for hiding this comment

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

@sr7cb hey Sanil, what kind of communication options would we expect to interface with in fftx? Trying to make sure our interface is generic enough

Copy link

Choose a reason for hiding this comment

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

Unfortunately I don't have enough familiarity with our MPI backend to provide a concrete answer. Please go ahead with your process and we can update/revisit down the road.

@@ -39,7 +39,8 @@ void calculateFFT( bool use_default, bool use_params,
Experimental::FastFourierTransformParams params;

// Set MPI communication
params.setAllToAll( true );
params.setAlltoAll( Cabana::Grid::Experimental::FFTCommPattern::alltoallv );
params.setAlltoAll( true );
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set with both?

Copy link
Author

Choose a reason for hiding this comment

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

You are right, only one is needed. I will remove the boolean variation. Its behavior and usage is commented at the example example/grid_tutorial/10_fft_heffte/heffte_fast_fourier_transform_example.cpp for backward compatibility

@rodrigoacb
Copy link
Author

Thanks for the improvements! Do you mind splitting into two PRs? I think I'll want someone else to at least co-review the spline details, but the FFT should be quick to merge

Hi @streeve Sam! We also thank you for maintaining Cabana! I don't mind splitting the PRs. Should I leave this one for the FFT and create a new one for the Splines?

I had a look into the actions, and I think I have already addressed it with fixes that I can commit once the PRs are at the places.

@streeve
Copy link
Member

streeve commented Nov 4, 2024

Thanks for the improvements! Do you mind splitting into two PRs? I think I'll want someone else to at least co-review the spline details, but the FFT should be quick to merge

Hi @streeve Sam! We also thank you for maintaining Cabana! I don't mind splitting the PRs. Should I leave this one for the FFT and create a new one for the Splines?

I had a look into the actions, and I think I have already addressed it with fixes that I can commit once the PRs are at the places.

Yes, that sounds great!

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.

3 participants