ECP-copa / Cabana

Performance-portable library for particle-based simulations

Other

197 stars 51 forks source link

heFFTe v1.0.1 API update #286

Closed af-ayala closed 3 years ago

af-ayala commented 3 years ago

Integration of heFFTe v 1.0.1 into Cabana ECP. The current API (fully based on C++11) shall remain for future developments.

codecov[bot] commented 3 years ago

Codecov Report

Merging #286 (0111752) into master (a40816b) will not change coverage. The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #286   +/-   ##
======================================
  Coverage    93.3%   93.3%           
======================================
  Files          36      36           
  Lines        2627    2627           
======================================
  Hits         2452    2452           
  Misses        175     175

Flag	Coverage Δ
clang	`93.3% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a40816b...0111752. Read the comment docs.

streeve commented 3 years ago

Sorry, @junghans I broke it again

sfogerty commented 3 years ago

@streeve @junghans My reasoning to build with Cabana_ENABLE_HEFFTE and not Cabana_REQUIRE_HEFFTE is in the details of Cabana_add_dependency(). If we set the REQUIRE var to ON, then we are calling find_package(Heffte) twice. Unfortunately Heffte v1.0 is not guarded against this, and a CMake error occurs. (My Heffte PR just a couple commits after v1.0)

streeve commented 3 years ago

Cabana_ENABLE_HEFFTE doesn't do anything, but I understand why you removed REQUIRE now. We'll need a minor release for that then

sfogerty commented 3 years ago

Cabana_ENABLE_HEFFTE doesn't do anything, but I understand why you removed REQUIRE now. We'll need a minor release for that then

Ah, right. I'm not sure there is an ideal solution at the moment. We could, for now, have travis just clone and build with the most recent heFFTe commit (not glued to v1.0 tag). Our build will start breaking when a new heFFTe version releases, but at that point we can redirect travis to build that new tagged version (v1.0.1, or whatever)?

(edit) nevermind - it looks like there was no real need for Cabana_add_dependency() to call find_package() two times. I rearranged it into an if-else and we should be able to avoid this issue, and then build heFFTe v1.0 just fine. :thumbsup:

streeve commented 3 years ago

This should be ready for another review

sslattery commented 3 years ago

You guys will need to do a rebase to resolve conflicts - they are likely from recent updates to the code formatting style.

sfogerty commented 3 years ago

Retest this please

streeve commented 3 years ago

I rebased to clean up the commit history.

Also just a note that the interpolation test failed again (2 ranks)

streeve commented 3 years ago

@junghans any idea why codecov is messed up?

junghans commented 3 years ago

Try to merge with master first.

streeve commented 3 years ago

Retest this please

streeve commented 3 years ago

This is ready for review (HIP failure is unrelated - not yet using HIP with FFT)

sslattery commented 3 years ago

retest this please

streeve commented 3 years ago

Retest this please

streeve commented 3 years ago

This is ready for (hopefully final) review