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

Enable FFT Mac projector #1381

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Enable FFT Mac projector #1381

wants to merge 16 commits into from

Conversation

marchdf
Copy link
Contributor

@marchdf marchdf commented Dec 3, 2024

Summary

Enable FFT Mac projector. Needs to go in after #1371.

Pull request type

Please check the type of change introduced:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Checklist

The following is included:

  • new unit-test(s)
  • new regression test(s)
  • documentation for new capability

This PR was tested by running:

  • the unit tests
    • on GPU
    • on CPU
  • the regression tests
    • on GPU
    • on CPU

Co-authored-by: prakash [email protected]
Thanks to @WeiqunZhang for the patch!

@marchdf
Copy link
Contributor Author

marchdf commented Dec 3, 2024

@moprak-nrel do you think you could test this branch with that case you were running?

@marchdf marchdf requested review from mbkuhn, jrood-nrel and moprak-nrel and removed request for mbkuhn December 3, 2024 22:58
@jrood-nrel
Copy link
Contributor

I think you will need to use target_compile_definitions for your preprocessor stuff.

@jrood-nrel
Copy link
Contributor

Or does AMReX have one you can use?

@marchdf
Copy link
Contributor Author

marchdf commented Dec 4, 2024

@WeiqunZhang does anyone parse a user option for m_use_fft? It defaults to true but I wonder if it makes sense to have the user be able to override that with mac_proj,use_fft ? Or is someone already parsing that (I couldn't find it).

@WeiqunZhang
Copy link
Contributor

No, there is no runtime parameter in amrex or amrex-hydro for using fft for mac projection. Here, m_use_fft is a member of amr-wind's own class. Yes, you can definitely make it a runtime parameter.

@moprak-nrel
Copy link
Contributor

I can confirm that with this PR, building amr-wind with fftw to enable the FFT mac projector is working.
Here is a the output from the abl_godunov regtest, with:
fft.txt
and without fft:
nofft.txt

Notice how fft.txt contains FFT::Poisson::solve vs nofft.txt which instead has MLPoisson::Fsmooth() and MLPoisson::Fapply().

Here is also the output from a single timestep,
with FFT:

Step: 1 dt: 0.5 Time: 0 to 0.5
CFL: 0.171317 (conv: 0.170223 diff: 0 src: 0.0136876 )

Godunov:
  System                     Iters      Initial residual        Final residual
  ----------------------------------------------------------------------------
  temperature_solve              1       9.600000794e-09       1.648459147e-12
  velocity_solve                 3        0.006048120402       4.014566457e-13
  Nodal_projection               7         0.01371718008       3.030952225e-14

WallClockTime: 1 Pre: 0.000742 Solve: 0.3553 Post: 0.00374 Total: 0.3598
Solve time per cell: 3.213e-06

without FFT:

Step: 1 dt: 0.5 Time: 0 to 0.5
CFL: 0.171317 (conv: 0.170223 diff: 0 src: 0.0136876 )

Godunov:
  System                     Iters      Initial residual        Final residual
  ----------------------------------------------------------------------------
  MAC_projection                 8       0.0001117207496       2.939258537e-15
  temperature_solve              1       9.600000794e-09       1.648459147e-12
  velocity_solve                 3        0.006048120402       4.014566457e-13
  Nodal_projection               7         0.01371718008       3.031038961e-14

WallClockTime: 1 Pre: 0.000717 Solve: 0.3754 Post: 0.00351 Total: 0.3796
Solve time per cell: 3.395e-06

@moprak-nrel
Copy link
Contributor

We need to decide how to proceed with the regtests/cdash; with this PR, by default, the MAC projections will use FFT (when applicable) when amr-wind is built with AMR_WIND_ENABLE_FFT=ON.

I had to explicitly add mac_proj.use_fft = 0 to disable FFT in my build.

@marchdf
Copy link
Contributor Author

marchdf commented Dec 4, 2024

Can you add that case as a regtest with the kind of guard we use for netcdf for example?

@moprak-nrel
Copy link
Contributor

As a quick verification test, I ran the burggraf flow reg test (with wenoz instead of ppm) and the error norms are identical:
burggraf_compare.pdf. The runtimes however are different
With FFT:

Time spent in InitData():    0.090864
Time spent in Evolve():      4.223058

Without FFT:

Time spent in InitData():    0.095678
Time spent in Evolve():      5.062773

@moprak-nrel moprak-nrel marked this pull request as ready for review December 5, 2024 17:13
m_fft_mac_proj = std::make_unique<Hydro::FFTMacProjector>(
m_repo.mesh().Geom(0), lobc, hibc);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like if the user has requested fft, but it is not possible to use, there is no warning or abort to notify the user that it is not being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, I can add a warning conditional on the input file having mac_proj.use_fft = 1. That would distinguish FFT being automatically enabled/disabled appropriately when AMR-Wind is compiled with FFT vs a user explicitly requesting to use FFT.

m_mac_proj->setUMAC(mac_vec);
#ifdef AMR_WIND_USE_FFT
if (m_fft_mac_proj) {
m_fft_mac_proj->setUMAC(mac_vec[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

did the usage of this change? the 0 index seems odd.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I guess this is because the FFT is restricted to single level. A comment may be instructive here, especially to avoid it getting accidentally copy-pasted in the future

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.

5 participants