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

ale3d #248

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

ale3d #248

wants to merge 192 commits into from

Conversation

ptsuji
Copy link
Collaborator

@ptsuji ptsuji commented Nov 28, 2023

Summary

  • This PR is a mergeback of ALE3D changes
    • Syncs with what's on the current ALE3D develop branch
    • Remove Windows Visual Studio files
    • Fix in SolidSPHHydroBase to turn off tensile forces when fragments are separating (allows "bouncing")
    • Fix for time step estimate due to velocity divergence in RZ space
    • Change some boost::unordered_maps to std::unordered_map or std::map to build with nvcc

AlbertNichols3 and others added 30 commits January 27, 2023 13:55
numbers as the fragment IDs. Using fragment IDs in evaluateDerivatives
to turn off forces between fragments when they are moving away from
each other.
This happens in FEusion/SPH when we update the smoothing lengths
with the scaling factor.
@jmikeowen
Copy link
Collaborator

But why wouldn't that tank our non-MPI build as well?

@ldowen
Copy link
Collaborator

ldowen commented Jun 28, 2024

We always include the Distributed source in our CMake system

DataBase
DataOutput
Distributed
FSISPH
Field

I think ALE3D was skipping that package altogether when MPI was turned off.

@ptsuji
Copy link
Collaborator Author

ptsuji commented Jul 19, 2024

@ptsuji To get this moving we need two things:

  1. To move the Communicator back to the Distributed package. Distributed has a sequential code path when MPI is disabled and should be used regardless of MPI status.
  2. The CMake macros / functions for Apple dynamic libraries.

@mdavis36 I think I've addressed both of these questions, let me know if I need to do anything else.

Copy link
Collaborator

@mdavis36 mdavis36 left a comment

Choose a reason for hiding this comment

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

Some comments on the CMake changes here, may want to re-review this once #278 is merged and the diff cleans up a bit

cmake/InstallTPLs.cmake Show resolved Hide resolved
cmake/SetupSpheral.cmake Outdated Show resolved Hide resolved
Comment on lines 2 to 32
#----------------------------------------------------------------------------------------
# Spheral_Handle_Ext
#----------------------------------------------------------------------------------------

# -------------------------------------------
# VARIABLES THAT NEED TO BE PREVIOUSLY DEFINED
# -------------------------------------------
# <lib_name>_DIR : REQUIRED : The installation location of the TPL
# <lib_name>_INCLUDES : OPTIONAL : Specific includes for the TPL

# ----------------------
# INPUT-OUTPUT VARIABLES
# ----------------------
# <lib_name> : REQUIRED : The name of the target TPL
# TPL_CMAKE_DIR : REQUIRED : Directory containing files for each TPL
# listing their library names

# -----------------------
# OUTPUT VARIABLES TO USE - Made available implicitly after function call
# -----------------------
# <lib_name> : Exportable target for the TPL
#----------------------------------------------------------------------------------------

function(Spheral_Handle_Ext lib_name libname APPLE)

if(APPLE)
set(SHARED_EXT "dylib")
set(${lib_name}_libs ${libname}.${SHARED_EXT})
endif()

endfunction()
Copy link
Collaborator

Choose a reason for hiding this comment

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

The documentation here doesn't match up with what this function is doing. This also seems to completely overwrites ${lib_name}_libs to contain only a single library in the final list. I think a function that searches ${lib_name}_libs for a set of libraries to be converted that finds and replaces them as necessary would be much more usefull.

Copy link
Collaborator Author

@ptsuji ptsuji Jul 24, 2024

Choose a reason for hiding this comment

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

I've rewritten this function to replace .dylib or .so extensions with .a if ENABLE_STATIC_TPL=On, and have updated the documentation.

cmake/tpl/polytope.cmake Outdated Show resolved Hide resolved
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.

None yet

10 participants