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

Value-View Interface #289

Open
wants to merge 53 commits into
base: develop
Choose a base branch
from
Open

Value-View Interface #289

wants to merge 53 commits into from

Conversation

mdavis36
Copy link
Collaborator

@mdavis36 mdavis36 commented Jul 23, 2024

Summary

  • This PR introduces the foundational features necessary for refactoring our current classes & objects in a way that is not too obstructive to the codebase.
    • A working ManagedVector (MV) implementation.
      • MV unit testing.
      • This currently lives within the Spheral headers. It will need to be refactored in the future and moved into CHAI with a better implementation.
    • The Value-View Interface Pattern (VVI) that is based around the experimental chai::ManagedSharedPtr.
      • Documentation on VVI in Spheral RTD. Preview here.
      • Example class patterns for:
        • Basic class w/ MV member.
        • CRTP pattern.
        • Abstract interface iheritance w/ MV member.
      • Implementation of VVI for Spheral::QuadraticInterpolator.
    • ctest c++ unit testing for ensuring semantic behavior of classes is working as expected with VVI.
      • Unit testing for Spheral::QuadraticInterpolator w/ and w/o VVI.
      • The start of unit tests for Spheral::Field. Incomplete
    • Gitlab CI:
      • make test at the end of the build stage.
      • SPHERAL_ENABLE_VVI=On for +cuda builds.

ToDo :

  • Annotate RELEASE_NOTES.md with notable changes.
  • Create LLNLSpheral PR pointing at this branch. (PR#105)
  • LLNLSpheral PR has passed all tests.

…; Moving SphArray.hh to Utilities/ManagedVector.hh
…d organization for VVI macros; Differentiating between Ptr and Ref syntax metaclasses with vvi pattern.
…o provide a clearer API for using VVI to users.
…Better default behavior for Value Interfaces, def copy-ctor, assign op and eq op behavior is baked in for free in the ValueInterface class.
…or defining default Ctor, Copy, Ass and Eq operations.
endif()
message("-----------------------------------------------------------------------------")
find_package(umpire REQUIRED NO_DEFAULT_PATH PATHS ${umpire_DIR})
if (umpire_FOUND)
message("Found umpire External Package.")
blt_convert_to_system_includes(TARGET umpire)
Copy link
Collaborator

@ptsuji ptsuji Sep 13, 2024

Choose a reason for hiding this comment

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

Should this blt_convert_to_system_includes() stuff also be in the block under "Found chai External Package" (and any other packages which could be found externally)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I`ll add it to CHAI. We already do this for all other external packages on line 127.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't do it for the packages we use find_package on. We only do it on packages we manually bring in. Was this necessary for your stuff?

Copy link
Collaborator Author

@mdavis36 mdavis36 Sep 13, 2024

Choose a reason for hiding this comment

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

It was, pedantic warnings from RAJA and umpire were causing our WARNINGS_AS_ERRORS builds to fail.

@@ -85,7 +90,10 @@ if(ENABLE_CUDA)
#set(CMAKE_CUDA_FLAGS "${CMAKE_CUDA_FLAGS} -arch=${CUDA_ARCH} --expt-relaxed-constexpr --extended-lambda -Xcudafe --display_error_number")
set(CMAKE_CUDA_STANDARD 17)
list(APPEND SPHERAL_CXX_DEPENDS cuda)
set(SPHERAL_ENABLE_CUDA On)
set(SPHERAL_ENABLE_VVI On)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the Value-View interface be enabled for CPU builds as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can be used for CPU builds but it is required for any GPU builds where we will be executing on the device. This just ensures it's enabled if you are building spheral w/ cuda.

src/CMakeLists.txt Outdated Show resolved Hide resolved
jmikeowen
jmikeowen previously approved these changes Sep 17, 2024
Copy link
Collaborator

@jmikeowen jmikeowen left a comment

Choose a reason for hiding this comment

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

Looking good once tests all pass

ldowen
ldowen previously approved these changes Oct 14, 2024
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.

4 participants