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

Feature python interface #53

Merged
merged 10 commits into from
Sep 19, 2024
Merged

Feature python interface #53

merged 10 commits into from
Sep 19, 2024

Conversation

ewanwm
Copy link
Owner

@ewanwm ewanwm commented Sep 18, 2024

Add a very basic python interface to nuTens. required quite a significant amount of rewriting particularly in Tensor code

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Cpp-linter Review

Only 5 out of 66 clang-tidy concerns fit within this pull request's diff.

Click here for the full clang-tidy patch
diff --git a/nuTens/propagator/const-density-solver.hpp b/nuTens/propagator/const-density-solver.hpp
index e6fb5d8..cb79bb1 100644
--- a/nuTens/propagator/const-density-solver.hpp
+++ b/nuTens/propagator/const-density-solver.hpp
@@ -83 +83 @@ class ConstDensityMatterSolver : public BaseMatterSolver
-    void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues);
+    void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues) override;
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index 0883b5a..46f867c 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -9,0 +10 @@
+#include <utility>
@@ -57 +58,2 @@ class Tensor
-    Tensor(){};
+    Tensor() = default;
+    ;
@@ -61 +63 @@ class Tensor
-    Tensor(const std::vector<float> &values, NTdtypes::scalarType type = NTdtypes::kFloat,
+    Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,
@@ -381 +383 @@ class Tensor
-        _tensor = tensor;
+        _tensor = std::move(tensor);
@@ -388 +390 @@ class Tensor
-    inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices) const
+    [[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices)
@@ -404 +406,2 @@ class Tensor
-    inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<Tensor::indexType> &indices) const
+    [[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(
+        const std::vector<Tensor::indexType> &indices)
diff --git a/nuTens/tensors/torch-tensor.cpp b/nuTens/tensors/torch-tensor.cpp
index a0c8c90..149441c 100644
--- a/nuTens/tensors/torch-tensor.cpp
+++ b/nuTens/tensors/torch-tensor.cpp
@@ -9 +9,2 @@ std::string Tensor::getTensorLibrary()
-Tensor::Tensor(std::vector<float> values, NTdtypes::scalarType type, NTdtypes::deviceType device, bool requiresGrad)
+Tensor::Tensor(const std::vector<float> &values, NTdtypes::scalarType type, NTdtypes::deviceType device,
+               bool requiresGrad)
diff --git a/python/pyNuTens.cpp b/python/pyNuTens.cpp
index dd81a81..b3f5d0e 100644
--- a/python/pyNuTens.cpp
+++ b/python/pyNuTens.cpp
@@ -17,3 +17,3 @@ namespace py = pybind11;
-void initTensor(py::module &);
-void initPropagator(py::module &);
-void initDtypes(py::module &);
+void initTensor(py::module & /*m*/);
+void initPropagator(py::module & /*m*/);
+void initDtypes(py::module & /*m*/);

Only 0 out of 1 clang-format concerns fit within this pull request's diff.

Click here for the full clang-format patch
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index 0883b5a..02613a8 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -61 +61 @@ class Tensor
-    Tensor(const std::vector<float> &values, NTdtypes::scalarType type = NTdtypes::kFloat,
+    Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,

Have any feedback or feature suggestions? Share it here.

@@ -44,7 +44,7 @@ Tensor Propagator::_calculateProbs(const Tensor &energies, const Tensor &massesS
{
for (int j = 0; j < _nGenerations; j++)
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostic

nuTens/propagator/propagator.cpp:45:23: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                      ^

clang-tidy diagnostic

nuTens/propagator/propagator.cpp:45:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                                                                        ^

clang-tidy diagnostic

nuTens/propagator/propagator.cpp:45:94: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                                                                                             ^

clang-tidy diagnostic

nuTens/propagator/propagator.hpp:42:35: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                  ^

clang-tidy diagnostic

nuTens/propagator/propagator.hpp:42:76: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                                                           ^

clang-tidy diagnostic

nuTens/propagator/propagator.hpp:42:97: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                                                                                ^

clang-tidy diagnostic

nuTens/propagator/propagator.hpp:45:23: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                      ^

clang-tidy diagnostic

nuTens/propagator/propagator.hpp:45:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                                                                        ^

clang-tidy diagnostic

nuTens/propagator/propagator.hpp:45:94: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                                                                                             ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:30:14: error: [clang-diagnostic-error]

use of undeclared identifier 'std'

const static std::map<scalarType, c10::ScalarType> scalarTypeMap = {{kFloat, torch::kFloat},
             ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:30:35: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<scalarType, c10::ScalarType> scalarTypeMap = {{kFloat, torch::kFloat},
                                  ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:30:78: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<scalarType, c10::ScalarType> scalarTypeMap = {{kFloat, torch::kFloat},
                                                                             ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:31:79: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                    {kDouble, torch::kDouble},
                                                                              ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:32:85: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                    {kComplexFloat, torch::kComplexFloat},
                                                                                    ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:33:86: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                    {kComplexDouble, torch::kComplexDouble}};
                                                                                     ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:36:14: error: [clang-diagnostic-error]

use of undeclared identifier 'std'

const static std::map<c10::ScalarType, scalarType> invScalarTypeMap = {{torch::kFloat, kFloat},
             ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:36:23: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<c10::ScalarType, scalarType> invScalarTypeMap = {{torch::kFloat, kFloat},
                      ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:36:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::ScalarType, scalarType> invScalarTypeMap = {{torch::kFloat, kFloat},
                                                                        ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:37:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                       {torch::kDouble, kDouble},
                                                                        ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:38:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                       {torch::kComplexFloat, kComplexFloat},
                                                                        ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:39:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                       {torch::kComplexDouble, kComplexDouble}};
                                                                        ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:42:14: error: [clang-diagnostic-error]

use of undeclared identifier 'std'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
             ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:42:35: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                  ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:42:76: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                                                           ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:42:97: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                                                                                ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:45:14: error: [clang-diagnostic-error]

use of undeclared identifier 'std'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
             ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:45:23: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                      ^

clang-tidy diagnostic

nuTens/tensors/dtypes.hpp:45:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                                                                        ^

clang-tidy diagnostic

nuTens/tensors/tensor.hpp:42:35: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                  ^

clang-tidy diagnostic

nuTens/tensors/tensor.hpp:42:76: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                                                           ^

clang-tidy diagnostic

nuTens/tensors/tensor.hpp:42:97: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                                                                                ^

clang-tidy diagnostic

nuTens/tensors/tensor.hpp:45:23: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                      ^

clang-tidy diagnostic

nuTens/tensors/tensor.hpp:45:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                                                                        ^

clang-tidy diagnostic

nuTens/tensors/tensor.hpp:45:94: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                                                                                             ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:30:35: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<scalarType, c10::ScalarType> scalarTypeMap = {{kFloat, torch::kFloat},
                                  ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:30:78: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<scalarType, c10::ScalarType> scalarTypeMap = {{kFloat, torch::kFloat},
                                                                             ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:31:79: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                    {kDouble, torch::kDouble},
                                                                              ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:32:85: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                    {kComplexFloat, torch::kComplexFloat},
                                                                                    ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:33:86: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                    {kComplexDouble, torch::kComplexDouble}};
                                                                                     ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:38:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                       {torch::kComplexFloat, kComplexFloat},
                                                                        ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:39:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                       {torch::kComplexDouble, kComplexDouble}};
                                                                        ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:42:35: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                  ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:42:76: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                                                           ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:42:97: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<deviceType, c10::DeviceType> deviceTypeMap = {{kCPU, torch::kCPU}, {kGPU, torch::kCUDA}};
                                                                                                ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:45:23: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                      ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:45:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                                                                        ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:45:94: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::DeviceType, deviceType> invDeviceTypeMap = {{torch::kCPU, kCPU}, {torch::kCUDA, kGPU}};
                                                                                             ^

clang-tidy diagnostic

python/pyNuTens.cpp:22:1: warning: [cppcoreguidelines-avoid-non-const-global-variables]

variable 'pybind11_module_def_pyNuTens' is non-const and globally accessible, consider making it const

PYBIND11_MODULE(pyNuTens, m)
^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:392:44: note: expanded from macro 'PYBIND11_MODULE'
    static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name)            \
                                           ^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:312:40: note: expanded from macro 'PYBIND11_CONCAT'
#define PYBIND11_CONCAT(first, second) first##second
                                       ^
note: expanded from here

clang-tidy diagnostic

python/pyNuTens.cpp:22:1: warning: [cppcoreguidelines-pro-bounds-pointer-arithmetic]

do not use pointer arithmetic

PYBIND11_MODULE(pyNuTens, m)
^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:397:9: note: expanded from macro 'PYBIND11_MODULE'
        PYBIND11_CHECK_PYTHON_VERSION                                                             \
        ^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:322:17: note: expanded from macro 'PYBIND11_CHECK_PYTHON_VERSION'
            || (runtime_ver[len] >= '0' && runtime_ver[len] <= '9')) {                            \
                ^

clang-tidy diagnostic

tests/tensor-basic.cpp:31:79: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                    {kDouble, torch::kDouble},
                                                                              ^

clang-tidy diagnostic

tests/tensor-basic.cpp:32:85: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                    {kComplexFloat, torch::kComplexFloat},
                                                                                    ^

clang-tidy diagnostic

tests/tensor-basic.cpp:33:86: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

                                                                    {kComplexDouble, torch::kComplexDouble}};
                                                                                     ^

clang-tidy diagnostic

tests/tensor-basic.cpp:36:14: error: [clang-diagnostic-error]

use of undeclared identifier 'std'

const static std::map<c10::ScalarType, scalarType> invScalarTypeMap = {{torch::kFloat, kFloat},
             ^

clang-tidy diagnostic

tests/tensor-basic.cpp:36:23: error: [clang-diagnostic-error]

use of undeclared identifier 'c10'

const static std::map<c10::ScalarType, scalarType> invScalarTypeMap = {{torch::kFloat, kFloat},
                      ^

clang-tidy diagnostic

tests/tensor-basic.cpp:36:73: error: [clang-diagnostic-error]

use of undeclared identifier 'torch'

const static std::map<c10::ScalarType, scalarType> invScalarTypeMap = {{torch::kFloat, kFloat},
                                                                        ^

{
NT_PROFILE();

_tensor = tensor;
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostics

Suggested change
_tensor = tensor;
_tensor = std::move(tensor);

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:160:9: error: [clang-diagnostic-error]

use of undeclared identifier 'NT_ERROR'

        NT_ERROR("Invalid dtype has been set for this tensor: {}", _dType);
        ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:161:9: error: [clang-diagnostic-error]

use of undeclared identifier 'NT_ERROR'

        NT_ERROR("{}:{}", __FILE__, __LINE__);
        ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/torch-tensor.cpp:205:12: warning: narrowing conversion from 'long' to signed type 'int' is implementation-defined [bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions]
    return _tensor.sizes()[0];
           ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/torch-tensor.cpp:215:18: warning: narrowing conversion from 'long' to signed type '__gnu_cxx::__alloc_traits<std::allocator<int>, int>::value_type' (aka 'int') is implementation-defined [bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions]
        ret[i] = _tensor.sizes()[i];
                 ^


/// Utility function to convert from a vector of ints to a vector of a10 tensor indices, which is needed for
/// accessing values of a tensor.
inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices) const
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostics

Suggested change
inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices) const
[[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices)


/// Utility function to convert from a vector of ints to a vector of a10 tensor indices, which is needed for
/// accessing values of a tensor.
inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<Tensor::indexType> &indices) const
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostics

Suggested change
inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<Tensor::indexType> &indices) const
[[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(
const std::vector<Tensor::indexType> &indices)

Comment on lines 17 to 19
void initTensor(py::module &);
void initPropagator(py::module &);
void initDtypes(py::module &);
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostics

Suggested change
void initTensor(py::module &);
void initPropagator(py::module &);
void initDtypes(py::module &);
void initTensor(py::module & /*m*/);
void initPropagator(py::module & /*m*/);
void initDtypes(py::module & /*m*/);

@github-actions github-actions bot dismissed their stale review September 18, 2024 18:12

outdated suggestion

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Cpp-linter Review

Only 2 out of 11 clang-tidy concerns fit within this pull request's diff.

Click here for the full clang-tidy patch
diff --git a/nuTens/propagator/const-density-solver.hpp b/nuTens/propagator/const-density-solver.hpp
index e6fb5d8..cb79bb1 100644
--- a/nuTens/propagator/const-density-solver.hpp
+++ b/nuTens/propagator/const-density-solver.hpp
@@ -83 +83 @@ class ConstDensityMatterSolver : public BaseMatterSolver
-    void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues);
+    void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues) override;
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index a4a57f4..902ebc4 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -58 +58,2 @@ class Tensor
-    Tensor(){};
+    Tensor() = default;
+    ;
@@ -62 +63 @@ class Tensor
-    Tensor(const std::vector<float> &values, NTdtypes::scalarType type = NTdtypes::kFloat,
+    Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,
@@ -389 +390 @@ class Tensor
-    [[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices) const
+    [[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices)
@@ -405,2 +406,2 @@ class Tensor
-    [[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(
-        const std::vector<Tensor::indexType> &indices) const
+    [[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(
+        const std::vector<Tensor::indexType> &indices)
diff --git a/nuTens/tensors/torch-tensor.cpp b/nuTens/tensors/torch-tensor.cpp
index a0c8c90..149441c 100644
--- a/nuTens/tensors/torch-tensor.cpp
+++ b/nuTens/tensors/torch-tensor.cpp
@@ -9 +9,2 @@ std::string Tensor::getTensorLibrary()
-Tensor::Tensor(std::vector<float> values, NTdtypes::scalarType type, NTdtypes::deviceType device, bool requiresGrad)
+Tensor::Tensor(const std::vector<float> &values, NTdtypes::scalarType type, NTdtypes::deviceType device,
+               bool requiresGrad)

Only 0 out of 1 clang-format concerns fit within this pull request's diff.

Click here for the full clang-format patch
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index a4a57f4..a05bd9d 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -62 +62 @@ class Tensor
-    Tensor(const std::vector<float> &values, NTdtypes::scalarType type = NTdtypes::kFloat,
+    Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,

Have any feedback or feature suggestions? Share it here.


/// Utility function to convert from a vector of ints to a vector of a10 tensor indices, which is needed for
/// accessing values of a tensor.
[[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices) const
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostics

Suggested change
[[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices) const
[[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices)

clang-tidy diagnostic

nuTens/tensors/tensor.hpp:383:48: warning: [bugprone-use-after-move]

'tensor' used after it was moved

        _dType = NTdtypes::invScalarTypeMap.at(tensor.scalar_type());
                                               ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/tensor.hpp:382:17: note: move occurred here
        _tensor = std::move(tensor);
                ^

clang-tidy diagnostic

nuTens/tensors/tensor.hpp:383:48: warning: [clang-analyzer-cplusplus.Move]

Method called on moved-from object 'tensor'

        _dType = NTdtypes::invScalarTypeMap.at(tensor.scalar_type());
                                               ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/tensor.hpp:382:9: note: Object 'tensor' is moved
        _tensor = std::move(tensor);
        ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/tensor.hpp:383:48: note: Method called on moved-from object 'tensor'
        _dType = NTdtypes::invScalarTypeMap.at(tensor.scalar_type());
                                               ^

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:58:5: warning: [clang-analyzer-optin.cplusplus.UninitializedObject]

2 uninitialized fields at the end of the constructor call

    Tensor(){};
    ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/tensor.hpp:342:26: note: uninitialized field 'this->_device'
    NTdtypes::deviceType _device;
                         ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/tensor.hpp:341:26: note: uninitialized field 'this->_dType'
    NTdtypes::scalarType _dType;
                         ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/torch-tensor.cpp:25:12: note: Calling default constructor for 'Tensor'
    Tensor ret;
           ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/tensor.hpp:58:5: note: 2 uninitialized fields at the end of the constructor call
    Tensor(){};
    ^

clang-tidy diagnostic

python/pyNuTens.cpp:22:1: warning: [cppcoreguidelines-avoid-non-const-global-variables]

variable 'pybind11_module_def_pyNuTens' is non-const and globally accessible, consider making it const

PYBIND11_MODULE(pyNuTens, m)
^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:392:44: note: expanded from macro 'PYBIND11_MODULE'
    static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name)            \
                                           ^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:312:40: note: expanded from macro 'PYBIND11_CONCAT'
#define PYBIND11_CONCAT(first, second) first##second
                                       ^
note: expanded from here

clang-tidy diagnostic

python/pyNuTens.cpp:22:1: warning: [cppcoreguidelines-pro-bounds-pointer-arithmetic]

do not use pointer arithmetic

PYBIND11_MODULE(pyNuTens, m)
^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:397:9: note: expanded from macro 'PYBIND11_MODULE'
        PYBIND11_CHECK_PYTHON_VERSION                                                             \
        ^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:322:17: note: expanded from macro 'PYBIND11_CHECK_PYTHON_VERSION'
            || (runtime_ver[len] >= '0' && runtime_ver[len] <= '9')) {                            \
                ^

Comment on lines 405 to 406
[[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(
const std::vector<Tensor::indexType> &indices) const
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostics

Suggested change
[[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(
const std::vector<Tensor::indexType> &indices) const
[[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(
const std::vector<Tensor::indexType> &indices)

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 64.28571% with 35 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nuTens/tensors/torch-tensor.cpp 56.00% 33 Missing ⚠️
nuTens/tensors/tensor.hpp 87.50% 1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Files with missing lines Coverage Δ
nuTens/propagator/const-density-solver.cpp 100.00% <100.00%> (ø)
nuTens/propagator/const-density-solver.hpp 100.00% <100.00%> (ø)
nuTens/propagator/propagator.cpp 65.00% <100.00%> (ø)
nuTens/propagator/propagator.hpp 42.85% <ø> (ø)
tests/tensor-basic.cpp 92.98% <100.00%> (ø)
nuTens/tensors/tensor.hpp 89.47% <87.50%> (-10.53%) ⬇️
nuTens/tensors/torch-tensor.cpp 58.54% <56.00%> (-6.62%) ⬇️

... and 1 file with indirect coverage changes

@github-actions github-actions bot dismissed their stale review September 18, 2024 19:02

outdated suggestion

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Cpp-linter Review

Only 3 out of 10 clang-tidy concerns fit within this pull request's diff.

Click here for the full clang-tidy patch
diff --git a/nuTens/propagator/const-density-solver.hpp b/nuTens/propagator/const-density-solver.hpp
index e6fb5d8..cb79bb1 100644
--- a/nuTens/propagator/const-density-solver.hpp
+++ b/nuTens/propagator/const-density-solver.hpp
@@ -83 +83 @@ class ConstDensityMatterSolver : public BaseMatterSolver
-    void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues);
+    void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues) override;
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index 2bb1705..2af868f 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -58 +58,2 @@ class Tensor
-    Tensor(){};
+    Tensor() = default;
+    ;
@@ -62 +63 @@ class Tensor
-    Tensor(const std::vector<float> &values, NTdtypes::scalarType type = NTdtypes::kFloat,
+    Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,
@@ -378 +379 @@ class Tensor
-    inline void setTensor(torch::Tensor tensor)
+    inline void setTensor(const torch::Tensor &tensor)
@@ -389 +390 @@ class Tensor
-    [[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices) const
+    [[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices)
@@ -405,2 +406,2 @@ class Tensor
-    [[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(
-        const std::vector<Tensor::indexType> &indices) const
+    [[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(
+        const std::vector<Tensor::indexType> &indices)
diff --git a/nuTens/tensors/torch-tensor.cpp b/nuTens/tensors/torch-tensor.cpp
index a0c8c90..149441c 100644
--- a/nuTens/tensors/torch-tensor.cpp
+++ b/nuTens/tensors/torch-tensor.cpp
@@ -9 +9,2 @@ std::string Tensor::getTensorLibrary()
-Tensor::Tensor(std::vector<float> values, NTdtypes::scalarType type, NTdtypes::deviceType device, bool requiresGrad)
+Tensor::Tensor(const std::vector<float> &values, NTdtypes::scalarType type, NTdtypes::deviceType device,
+               bool requiresGrad)

Only 0 out of 1 clang-format concerns fit within this pull request's diff.

Click here for the full clang-format patch
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index 2bb1705..8dc02fe 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -62 +62 @@ class Tensor
-    Tensor(const std::vector<float> &values, NTdtypes::scalarType type = NTdtypes::kFloat,
+    Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,

Have any feedback or feature suggestions? Share it here.

NT_PROFILE();

return _tensor;
}

private:
/// Set the underlying tensor, setting the relevant information like _dtype and _device
inline void setTensor(torch::Tensor tensor)
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostics

Suggested change
inline void setTensor(torch::Tensor tensor)
inline void setTensor(const torch::Tensor &tensor)

clang-tidy diagnostic

nuTens/tensors/torch-tensor.cpp:58:5: warning: [clang-analyzer-optin.cplusplus.UninitializedObject]

2 uninitialized fields at the end of the constructor call

    Tensor(){};
    ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/tensor.hpp:342:26: note: uninitialized field 'this->_device'
    NTdtypes::deviceType _device;
                         ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/tensor.hpp:341:26: note: uninitialized field 'this->_dType'
    NTdtypes::scalarType _dType;
                         ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/torch-tensor.cpp:25:12: note: Calling default constructor for 'Tensor'
    Tensor ret;
           ^
/home/runner/work/nuTens/nuTens/nuTens/tensors/tensor.hpp:58:5: note: 2 uninitialized fields at the end of the constructor call
    Tensor(){};
    ^

clang-tidy diagnostic

python/pyNuTens.cpp:22:1: warning: [cppcoreguidelines-avoid-non-const-global-variables]

variable 'pybind11_module_def_pyNuTens' is non-const and globally accessible, consider making it const

PYBIND11_MODULE(pyNuTens, m)
^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:392:44: note: expanded from macro 'PYBIND11_MODULE'
    static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name)            \
                                           ^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:312:40: note: expanded from macro 'PYBIND11_CONCAT'
#define PYBIND11_CONCAT(first, second) first##second
                                       ^
note: expanded from here

clang-tidy diagnostic

python/pyNuTens.cpp:22:1: warning: [cppcoreguidelines-pro-bounds-pointer-arithmetic]

do not use pointer arithmetic

PYBIND11_MODULE(pyNuTens, m)
^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:397:9: note: expanded from macro 'PYBIND11_MODULE'
        PYBIND11_CHECK_PYTHON_VERSION                                                             \
        ^
/home/runner/.local/lib/python3.10/site-packages/torch/include/pybind11/detail/common.h:322:17: note: expanded from macro 'PYBIND11_CHECK_PYTHON_VERSION'
            || (runtime_ver[len] >= '0' && runtime_ver[len] <= '9')) {                            \
                ^


/// Utility function to convert from a vector of ints to a vector of a10 tensor indices, which is needed for
/// accessing values of a tensor.
[[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices) const
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostics

Suggested change
[[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices) const
[[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(const std::vector<int> &indices)

Comment on lines 405 to 406
[[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(
const std::vector<Tensor::indexType> &indices) const
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-tidy diagnostics

Suggested change
[[nodiscard]] inline std::vector<at::indexing::TensorIndex> convertIndices(
const std::vector<Tensor::indexType> &indices) const
[[nodiscard]] static inline std::vector<at::indexing::TensorIndex> convertIndices(
const std::vector<Tensor::indexType> &indices)

Copy link
Contributor

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy reports: 5 concern(s)
  • benchmarks/benchmarks.cpp:2:10: error: [clang-diagnostic-error]

    'benchmark/benchmark.h' file not found

    #include <benchmark/benchmark.h>
             ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:61:29: warning: 0.1 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor masses = Tensor({0.1, 0.2, 0.3}, NTdtypes::kFloat).requiresGrad(false).addBatchDim();
                                ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:61:34: warning: 0.2 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor masses = Tensor({0.1, 0.2, 0.3}, NTdtypes::kFloat).requiresGrad(false).addBatchDim();
                                     ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:61:39: warning: 0.3 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor masses = Tensor({0.1, 0.2, 0.3}, NTdtypes::kFloat).requiresGrad(false).addBatchDim();
                                          ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:63:30: warning: 0.23 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor theta23 = Tensor({0.23}).dType(NTdtypes::kComplexFloat).requiresGrad(false);
                                 ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:64:30: warning: 0.13 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor theta13 = Tensor({0.13}).dType(NTdtypes::kComplexFloat).requiresGrad(false);
                                 ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:65:30: warning: 0.12 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor theta12 = Tensor({0.12}).dType(NTdtypes::kComplexFloat).requiresGrad(false);
                                 ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:66:30: warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor deltaCP = Tensor({0.5}).dType(NTdtypes::kComplexFloat).requiresGrad(false);
                                 ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:76:16: warning: 123 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        std::srand(123);
                   ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:89:29: warning: 0.1 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor masses = Tensor({0.1, 0.2, 0.3}, NTdtypes::kFloat).requiresGrad(false).addBatchDim();
                                ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:89:34: warning: 0.2 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor masses = Tensor({0.1, 0.2, 0.3}, NTdtypes::kFloat).requiresGrad(false).addBatchDim();
                                     ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:89:39: warning: 0.3 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor masses = Tensor({0.1, 0.2, 0.3}, NTdtypes::kFloat).requiresGrad(false).addBatchDim();
                                          ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:91:30: warning: 0.23 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor theta23 = Tensor({0.23}).dType(NTdtypes::kComplexFloat).requiresGrad(false);
                                 ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:92:30: warning: 0.13 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor theta13 = Tensor({0.13}).dType(NTdtypes::kComplexFloat).requiresGrad(false);
                                 ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:93:30: warning: 0.12 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor theta12 = Tensor({0.12}).dType(NTdtypes::kComplexFloat).requiresGrad(false);
                                 ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:94:30: warning: 0.5 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        Tensor deltaCP = Tensor({0.5}).dType(NTdtypes::kComplexFloat).requiresGrad(false);
                                 ^
    /home/runner/work/nuTens/nuTens/benchmarks/benchmarks.cpp:106:16: warning: 123 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers]
        std::srand(123);
                   ^
  • benchmarks/benchmarks.cpp:116:11: warning: [cppcoreguidelines-avoid-non-const-global-variables]

    variable 'BM_vacuumOscillations' is non-const and globally accessible, consider making it const

    BENCHMARK(BM_vacuumOscillations)->Name("Vacuum Oscillations")->Args({1 << 10, 1 << 10});
              ^
  • benchmarks/benchmarks.cpp:119:11: warning: [cppcoreguidelines-avoid-non-const-global-variables]

    variable 'BM_constMatterOscillations' is non-const and globally accessible, consider making it const

    BENCHMARK(BM_constMatterOscillations)->Name("Const Density Oscillations")->Args({1 << 10, 1 << 10});
              ^
  • nuTens/propagator/const-density-solver.hpp:83:10: warning: [clang-diagnostic-inconsistent-missing-override]

    'calculateEigenvalues' overrides a member function but is not marked 'override'

        void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues);
             ^
    /home/runner/work/nuTens/nuTens/nuTens/propagator/base-matter-solver.hpp:20:18: note: overridden virtual function is here
        virtual void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues) = 0;
                     ^
    /home/runner/work/nuTens/nuTens/nuTens/propagator/const-density-solver.hpp:83:10: warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions,modernize-use-override]
        void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues);
             ^
                                                                                                     override
  • nuTens/tensors/torch-tensor.cpp:9:35: warning: [performance-unnecessary-value-param]

    the parameter 'values' is copied for each invocation but only used as a const reference; consider making it a const reference

    Tensor::Tensor(std::vector<float> values, NTdtypes::scalarType type, NTdtypes::deviceType device, bool requiresGrad)
                                      ^
                   const             &

Have any feedback or feature suggestions? Share it here.

@github-actions github-actions bot dismissed their stale review September 19, 2024 03:07

outdated suggestion

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Cpp-linter Review

Click here for the full clang-tidy patch
diff --git a/nuTens/propagator/const-density-solver.hpp b/nuTens/propagator/const-density-solver.hpp
index e6fb5d8..cb79bb1 100644
--- a/nuTens/propagator/const-density-solver.hpp
+++ b/nuTens/propagator/const-density-solver.hpp
@@ -83 +83 @@ class ConstDensityMatterSolver : public BaseMatterSolver
-    void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues);
+    void calculateEigenvalues(const Tensor &energies, Tensor &eigenvectors, Tensor &eigenvalues) override;
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index 3ab4c84..438e8c7 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -66 +66 @@ class Tensor
-    Tensor(const std::vector<float> &values, NTdtypes::scalarType type = NTdtypes::kFloat,
+    Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,
diff --git a/nuTens/tensors/torch-tensor.cpp b/nuTens/tensors/torch-tensor.cpp
index a0c8c90..149441c 100644
--- a/nuTens/tensors/torch-tensor.cpp
+++ b/nuTens/tensors/torch-tensor.cpp
@@ -9 +9,2 @@ std::string Tensor::getTensorLibrary()
-Tensor::Tensor(std::vector<float> values, NTdtypes::scalarType type, NTdtypes::deviceType device, bool requiresGrad)
+Tensor::Tensor(const std::vector<float> &values, NTdtypes::scalarType type, NTdtypes::deviceType device,
+               bool requiresGrad)
Click here for the full clang-format patch
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index 3ab4c84..438e8c7 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -66 +66 @@ class Tensor
-    Tensor(const std::vector<float> &values, NTdtypes::scalarType type = NTdtypes::kFloat,
+    Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,

Have any feedback or feature suggestions? Share it here.

Copy link
Contributor

🐰 Bencher Report

Branchfeature_python_interface
Testbedubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencynanoseconds (ns)
Const Density Oscillations/1024/1024📈 view plot
⚠️ NO THRESHOLD
4,729,412,983.00
Const Density Oscillations/1024/1024_cv📈 view plot
⚠️ NO THRESHOLD
0.01
Const Density Oscillations/1024/1024_mean📈 view plot
⚠️ NO THRESHOLD
4,693,651,480.88
Const Density Oscillations/1024/1024_median📈 view plot
⚠️ NO THRESHOLD
4,681,191,727.50
Const Density Oscillations/1024/1024_stddev📈 view plot
⚠️ NO THRESHOLD
27,201,911.95
Vacuum Oscillations/1024/1024📈 view plot
⚠️ NO THRESHOLD
266,803,669.67
Vacuum Oscillations/1024/1024_cv📈 view plot
⚠️ NO THRESHOLD
0.00
Vacuum Oscillations/1024/1024_mean📈 view plot
⚠️ NO THRESHOLD
265,933,563.46
Vacuum Oscillations/1024/1024_median📈 view plot
⚠️ NO THRESHOLD
265,969,105.83
Vacuum Oscillations/1024/1024_stddev📈 view plot
⚠️ NO THRESHOLD
893,476.35
🐰 View full continuous benchmarking report in Bencher

@ewanwm ewanwm merged commit 8b8c1cb into main Sep 19, 2024
3 of 5 checks passed
@ewanwm ewanwm deleted the feature_python_interface branch September 19, 2024 03:16
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.

2 participants