Merge pull request #388 from seoklab/fix-crdgen

fix(algo/crdgen): handle edge cases more gracefully

CMakeLists.txt

@@ -34,9 +34,8 @@ elseif(NOT CMAKE_CXX_COMPILER_ID MATCHES "GNU")
     "Please use clang or gcc as a compiler.")
 endif()
 
-set(CMAKE_CXX_STANDARD 17)
-set(CMAKE_CXX_EXTENSIONS OFF)
-set(CMAKE_CXX_STANDARD_REQUIRED ON)
+# for math optimization flags
+string(APPEND CMAKE_CXX_FLAGS " -std=c++17")
 set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 
 set(NURI_GLOBAL_OPTFLAGS " -${NURI_OPTIMIZATION_LEVEL}")

cmake/NuriKitUtils.cmake

@@ -86,6 +86,12 @@ function(nuri_get_version)
   set(NURI_REF "${NURI_REF}" PARENT_SCOPE)
 endfunction()
 
+macro(nuri_set_cxx_standard)
+  set(CMAKE_CXX_STANDARD 17)
+  set(CMAKE_CXX_EXTENSIONS OFF)
+  set(CMAKE_CXX_STANDARD_REQUIRED ON)
+endmacro()
+
 function(nuri_make_available_deponly target)
   include(FetchContent)
 
@@ -111,6 +117,8 @@ function(target_system_include_directories target)
 endfunction()
 
 function(find_or_fetch_eigen)
+  nuri_set_cxx_standard()
+
   set(CMAKE_POLICY_DEFAULT_CMP0077 NEW)
   set(BUILD_TESTING OFF)
   set(EIGEN_BUILD_DOC OFF)
@@ -133,6 +141,7 @@ function(find_or_fetch_eigen)
 endfunction()
 
 function(find_or_fetch_spectra)
+  nuri_set_cxx_standard()
   set(BUILD_TESTING OFF)
 
   find_package(Spectra 1.0 QUIET)
@@ -153,6 +162,7 @@ function(find_or_fetch_spectra)
 endfunction()
 
 function(find_or_fetch_pybind11)
+  nuri_set_cxx_standard()
   set(BUILD_TESTING OFF)
 
   # FindPythonInterp/FindPythonLibs deprecated since cmake 3.12
@@ -178,6 +188,7 @@ function(find_or_fetch_pybind11)
 endfunction()
 
 function(handle_boost_dependency target)
+  nuri_set_cxx_standard()
   set(BUILD_TESTING OFF)
 
   # FindBoost deprecated since cmake 3.30

include/nuri/algo/crdgen.h

@@ -20,24 +20,29 @@ namespace nuri {
  * @param mol The molecule to generate coordinates.
  * @param conf A matrix to store the generated coordinates.
  * @param max_trial The maximum number of trials to generate trial distances.
+ * @param seed The seed for the random number generator. Might not be used if
+ *        algorithm succeeds in the first trial.
  * @return Whether the coordinates is successfully generated.
  */
 extern bool generate_coords(const Molecule &mol, Matrix3Xd &conf,
-                            int max_trial = 10);
+                            int max_trial = 10, int seed = 0);
 
 /**
  * @brief Generate 3D coordinates for the molecule.
  *
  * @param mol The molecule to generate coordinates.
+ * @param max_trial The maximum number of trials to generate trial distances.
+ * @param seed The seed for the random number generator. Might not be used if
+ *        algorithm succeeds in the first trial.
  * @return Whether the coordinates is successfully generated.
  *
  * @note The generated coordinates is stored as the last conformer of the
  *       molecule, if and only if the coordinates is successfully generated. The
  *       molecule is not modified otherwise.
  */
-inline bool generate_coords(Molecule &mol, int max_trial = 10) {
+inline bool generate_coords(Molecule &mol, int max_trial = 10, int seed = 0) {
   Matrix3Xd conf(3, mol.num_atoms());
-  bool success = generate_coords(mol, conf, max_trial);
+  bool success = generate_coords(mol, conf, max_trial, seed);
   if (success)
     mol.confs().push_back(std::move(conf));
   return success;

include/nuri/algo/guess.h

@@ -9,7 +9,7 @@
 #include "nuri/core/molecule.h"
 
 namespace nuri {
-constexpr inline double kDefaultThreshold = 0.5;
+constexpr double kDefaultThreshold = 0.5;
 
 /**
  * @brief Guess bonds, types of atoms, and number of hydrogens of a molecule.

include/nuri/core/bool_matrix.h

@@ -20,8 +20,7 @@
 namespace nuri {
 namespace internal {
   using Block = uint64_t;
-  constexpr inline Eigen::Index kBitsPerBlock =
-      std::numeric_limits<Block>::digits;
+  constexpr Eigen::Index kBitsPerBlock = std::numeric_limits<Block>::digits;
 }  // namespace internal
 
 class BoolMatrixKey {

include/nuri/core/element.h

@@ -21,7 +21,7 @@
 namespace nuri {
 namespace internal {
   // Value taken from https://physics.nist.gov/cgi-bin/cuu/Value?are
-  extern constexpr inline double kElectronMass = 5.48579909065e-4;
+  constexpr double kElectronMass = 5.48579909065e-4;
 }  // namespace internal
 
 struct Isotope {

include/nuri/core/geometry.h

@@ -88,38 +88,38 @@ class OCTree {
 };
 
 namespace constants {
-  extern constexpr inline double kPi =
+  constexpr double kPi =
       3.1415926535897932384626433832795028841971693993751058209749445923078164;
 
-  extern constexpr inline double kTwoPi =
+  constexpr double kTwoPi =
       6.2831853071795864769252867665590057683943387987502116419498891846156328;
 
   // NOLINTBEGIN(*-identifier-naming)
-  extern constexpr inline double kCos15 =
+  constexpr double kCos15 =
       0.9659258262890682867497431997288973676339048390084045504023430763;
-  extern constexpr inline double kCos75 =
+  constexpr double kCos75 =
       0.2588190451025207623488988376240483283490689013199305138140032073;
-  extern constexpr inline double kCos100 =
+  constexpr double kCos100 =
       -0.173648177666930348851716626769314796000375677184069387236241378;
-  extern constexpr inline double kCos102 =
+  constexpr double kCos102 =
       -0.207911690817759337101742284405125166216584760627723836407181973;
-  extern constexpr inline double kCos112 =
+  constexpr double kCos112 =
       -0.374606593415912035414963774501195131000158922253676174103440371;
-  extern constexpr inline double kCos115 =
+  constexpr double kCos115 =
       -0.422618261740699436186978489647730181563129301194864623444415159;
-  extern constexpr inline double kCos125 =
+  constexpr double kCos125 =
       -0.573576436351046096108031912826157864620433371450986351081027118;
-  extern constexpr inline double kCos155 =
+  constexpr double kCos155 =
       -0.906307787036649963242552656754316983267712625175864680871298408;
-  extern constexpr inline double kCos175 =
+  constexpr double kCos175 =
       -0.996194698091745532295010402473888046183562672645850974525442277;
-  extern constexpr inline double kTan10_2 =
+  constexpr double kTan10_2 =
       0.0874886635259240052220186694349614581194542763681082291452366622;
-  extern constexpr inline double kTan15_2 =
+  constexpr double kTan15_2 =
       0.1316524975873958534715264574097171035928141022232375735535653257;
-  extern constexpr inline double kTan116_2 =
+  constexpr double kTan116_2 =
       1.6003345290410503553267330811833575255040718469227591484115002297;
-  extern constexpr inline double kTan155_2 =
+  constexpr double kTan155_2 =
       4.5107085036620571342899391172547519686713241944553043587162345185;
   // NOLINTEND(*-identifier-naming)
 }  // namespace constants

include/nuri/core/molecule.h

@@ -102,8 +102,7 @@ namespace constants {
     return os << "other";
   }
 
-  extern constexpr inline double kBondOrderToDouble[] = { 0.0, 1.0, 2.0,
-                                                          3.0, 4.0, 1.5 };
+  constexpr double kBondOrderToDouble[] = { 0.0, 1.0, 2.0, 3.0, 4.0, 1.5 };
 }  // namespace constants
 
 inline constants::Hybridization clamp_hyb(int hyb) {

include/nuri/nurikit_config.h.in

@@ -17,8 +17,10 @@
 #endif
 
 namespace nuri {
-constexpr inline std::string_view kNuriVersion = "@NURI_VERSION@";
-constexpr inline std::string_view kNuriFullVersion = "@NURI_FULL_VERSION@";
+// NOLINTBEGIN(clang-diagnostic-unused-const-variable)
+constexpr std::string_view kNuriVersion = "@NURI_VERSION@";
+constexpr std::string_view kNuriFullVersion = "@NURI_FULL_VERSION@";
+// NOLINTEND(clang-diagnostic-unused-const-variable)
 }  // namespace nuri
 
 #endif /* NURI_NURIKIT_CONFIG_H_ */

include/nuri/utils.h

@@ -632,7 +632,7 @@ namespace internal {
   }
 
   // RDKit-compatible key for name
-  constexpr inline std::string_view kNameKey = "_Name";
+  constexpr std::string_view kNameKey = "_Name";
 
   template <class PT>
   auto find_key(PT &props, std::string_view key) {

python/include/nuri/python/utils.h

@@ -323,8 +323,8 @@ inline int py_check_index(int size, int idx, const char *onerror) {
   return idx;
 }
 
-constexpr inline static py::keep_alive<0, 1> kReturnsSubobject {};
-constexpr inline static py::call_guard<py::gil_scoped_release> kThreadSafe {};
+constexpr py::keep_alive<0, 1> kReturnsSubobject {};
+constexpr py::call_guard<py::gil_scoped_release> kThreadSafe {};
 
 template <class T, class Size, class Getter, class Iter>
 py::class_<T> &add_sequence_interface(py::class_<T> &cls, Size size,

python/src/nuri/_log_interface.cpp

@@ -15,11 +15,10 @@
 #include <absl/log/initialize.h>
 #include <absl/log/log_entry.h>
 #include <absl/log/log_sink.h>
+#include <absl/log/log_sink_registry.h>
 #include <absl/log/vlog_is_on.h>
 
-#ifndef __clang_analyzer__
-#include <absl/log/log_sink_registry.h>
-#endif
+#include "nuri/meta.h"
 
 namespace nuri {
 namespace python_internal {
@@ -90,11 +89,8 @@ class PyLogSink: public absl::LogSink {
       logger_ = py::module_::import("logging").attr("getLogger")("nuri");
       logger_.inc_ref();
 
-      // Why no nolint for clang static analyzer?
-#ifndef __clang_analyzer__
       // NOLINTNEXTLINE(*-owning-memory)
-      absl::AddLogSink(new PyLogSink);
-#endif
+      NURI_CLANG_ANALYZER_NOLINT absl::AddLogSink(new PyLogSink);
       absl::SetStderrThreshold(absl::LogSeverityAtLeast::kFatal);
       set_log_level(10);
       ABSL_DLOG(INFO) << "initialized NuriKit Python logging sink.";

python/src/nuri/algo/algo_module.cpp

@@ -319,15 +319,16 @@ void bind_crdgen(py::module_ &m) {
 
   m.def(
       "generate_coords",
-      [](PyMol &mol, std::string_view method, int trial) {
+      [](PyMol &mol, std::string_view method, int trial, int seed) {
         if (absl::AsciiStrToUpper(method) != "DG")
           throw py::value_error(absl::StrCat("Unsupported method: ", method));
 
-        bool success = generate_coords(*mol, trial);
+        bool success = generate_coords(*mol, trial, seed);
         if (!success)
           throw py::value_error("Failed to generate coordinates");
       },
       py::arg("mol"), py::arg("method") = "DG", py::arg("max_trial") = 10,
+      py::arg("seed") = 0,
       R"doc(
 Generate 3D coordinates of a molecule. The generated coordinates are stored in
 the last conformer of the molecule if the generation is successful.
@@ -336,6 +337,9 @@ the last conformer of the molecule if the generation is successful.
 :param method: The method to use for coordinate generation (case insensitive).
   Currently, only ``DG`` (distance geometry) is supported.
 :param max_trial: The maximum number of trials to generate trial distances.
+:param seed: The seed for the random number generator. Might not be used
+  depending on the method used or if the algorithm succeeds before random
+  initialization.
 :raises ValueError: If the generation fails. On exception, the molecule is left
   unmodified.
 )doc");

python/test/algo/crdgen_test.py

@@ -26,21 +26,21 @@ def test_crdgen_distgeom(sample: Molecule):
     assert np.allclose(
         conf,
         [
-            [-2.660, 1.200, 0.539],
-            [-1.787, 0.051, 0.036],
-            [-1.671, -0.981, 0.715],
-            [-1.188, 0.148, -1.095],
-            [0.111, 0.683, -1.280],
-            [0.163, 2.150, -0.882],
-            [0.144, 2.286, 0.634],
-            [1.209, 1.469, 1.313],
-            [1.509, 0.149, 0.783],
-            [0.970, -1.042, 1.575],
-            [0.686, -2.046, 0.589],
-            [0.840, -1.641, -0.629],
-            [0.469, -2.281, -1.618],
-            [1.205, -0.144, -0.679],
+            [-2.935, 1.012, 0.302],
+            [-1.763, 0.055, 0.158],
+            [-1.909, -1.037, 0.717],
+            [-1.160, 0.101, -0.966],
+            [0.103, 0.678, -1.212],
+            [0.163, 2.143, -0.808],
+            [0.313, 2.278, 0.700],
+            [1.506, 1.506, 1.230],
+            [1.613, 0.105, 0.744],
+            [0.757, -0.897, 1.489],
+            [0.689, -1.977, 0.565],
+            [0.957, -1.626, -0.639],
+            [0.404, -2.209, -1.577],
+            [1.262, -0.131, -0.703],
         ],
-        atol=1e-2,
+        atol=5e-2,
         rtol=0,
     )

src/CMakeLists.txt

@@ -28,13 +28,28 @@ target_link_libraries(nuri_lib
   PUBLIC
   absl::strings
   absl::flat_hash_map
-  absl::random_random
   absl::absl_check
   absl::absl_log
 )
 target_system_include_directories(nuri_lib Eigen3::Eigen Spectra::Spectra)
 handle_boost_dependency(nuri_lib)
 
+if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
+  list(APPEND NURI_LIBRARY_FLAGS
+    -fno-math-errno
+    -fno-signed-zeros
+    -fno-trapping-math
+    -fassociative-math
+    -freciprocal-math
+    -fno-rounding-math
+    -ffp-contract=fast
+  )
+
+  if(CMAKE_CXX_COMPILER_ID MATCHES "GNU")
+    list(APPEND NURI_LIBRARY_FLAGS -fno-signaling-nans -fexcess-precision=fast)
+  endif()
+endif()
+
 if(NURI_LIBRARY_FLAGS)
   target_compile_options(nuri_lib PRIVATE ${NURI_LIBRARY_FLAGS})
   target_link_options(nuri_lib PRIVATE ${NURI_LIBRARY_FLAGS})
@@ -45,7 +60,7 @@ set_target_properties(nuri_exe
   PROPERTIES
   OUTPUT_NAME nuri
   INSTALL_RPATH "$ORIGIN/../${CMAKE_INSTALL_LIBDIR}")
-target_link_libraries(nuri_exe PRIVATE nuri_lib)
+target_link_libraries(nuri_exe PRIVATE nuri_lib absl::log_initialize)
 
 install(TARGETS nuri_lib LIBRARY)
 install(TARGETS nuri_exe RUNTIME)