Feature improve tensor interface #41
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ault one every time before building them
…plit up the declaration and initialisation
… tensors less clunky
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cpp-linter Review
Click here for the full clang-format patch
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index e3a587a..2b0eaac 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -56 +56 @@ class Tensor
- Tensor(const std::vector<float>& values, NTdtypes::scalarType type = NTdtypes::kFloat,
+ Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,
Only 3 out of 9 clang-tidy concerns fit within this pull request's diff.
Click here for the full clang-tidy patch
diff --git a/benchmarks/benchmarks.cpp b/benchmarks/benchmarks.cpp
index ebd71ca..cb6f4c3 100644
--- a/benchmarks/benchmarks.cpp
+++ b/benchmarks/benchmarks.cpp
@@ -99 +99 @@ static void BM_constMatterOscillations(benchmark::State &state)
- std::unique_ptr<BaseMatterSolver> matterSolver = std::make_unique<ConstDensityMatterSolver>(3, 2.6);
+ std::unique_ptr<BaseMatterSolver> matterSolver = 0 = std::make_unique<ConstDensityMatterSolver>(3, 2.6);
diff --git a/nuTens/propagator/const-density-solver.hpp b/nuTens/propagator/const-density-solver.hpp
index 5aa5b34..19470ea 100644
--- a/nuTens/propagator/const-density-solver.hpp
+++ b/nuTens/propagator/const-density-solver.hpp
@@ -82 +82 @@ 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 e3a587a..f825ea5 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -52 +52 @@ class Tensor
- Tensor(){};
+ Tensor()= default;;
@@ -56 +56 @@ 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 3324317..cf91698 100644
--- a/nuTens/tensors/torch-tensor.cpp
+++ b/nuTens/tensors/torch-tensor.cpp
@@ -20 +20 @@ 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)
Have any feedback or feature suggestions? Share it here.
|
|
||
| /// @brief Construct a 1-d array with specified values | ||
| /// @arg values The values to include in the tensor | ||
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, |
There was a problem hiding this comment.
clang-format suggestions
Suggested change
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, | |
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, |
| bool requiresGrad = true); | ||
| /// @brief Initialise this tensor with ones | ||
| /// @brief Default constructor with no initialisation | ||
| Tensor(){}; |
There was a problem hiding this comment.
clang-tidy diagnostics
- use '= default' to define a trivial default constructor [modernize-use-equals-default]
Suggested change
| Tensor(){}; | |
| Tensor()= default;; |
|
|
||
| /// @brief Construct a 1-d array with specified values | ||
| /// @arg values The values to include in the tensor | ||
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, |
There was a problem hiding this comment.
clang-tidy suggestions
Suggested change
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, | |
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, |
| @@ -17,45 +17,72 @@ std::string Tensor::getTensorLibrary() | |||
| return "PyTorch"; | |||
| } | |||
|
|
|||
| Tensor &Tensor::ones(int length, NTdtypes::scalarType type, NTdtypes::deviceType device, bool requiresGrad) | |||
| Tensor::Tensor(std::vector<float> values, NTdtypes::scalarType type, NTdtypes::deviceType device, bool requiresGrad) | |||
There was a problem hiding this comment.
clang-tidy diagnostics
- the parameter 'values' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
Suggested change
| 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) |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
|
…sity matter solver
Cpp-Linter Report
|
There was a problem hiding this comment.
Cpp-linter Review
Click here for the full clang-format patch
diff --git a/nuTens/tensors/tensor.hpp b/nuTens/tensors/tensor.hpp
index e3a587a..2b0eaac 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -56 +56 @@ class Tensor
- Tensor(const std::vector<float>& values, NTdtypes::scalarType type = NTdtypes::kFloat,
+ Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat,
Only 3 out of 9 clang-tidy concerns fit within this pull request's diff.
Click here for the full clang-tidy patch
diff --git a/benchmarks/benchmarks.cpp b/benchmarks/benchmarks.cpp
index 2f729d2..87fcbe1 100644
--- a/benchmarks/benchmarks.cpp
+++ b/benchmarks/benchmarks.cpp
@@ -100 +100 @@ static void BM_constMatterOscillations(benchmark::State &state)
- std::unique_ptr<BaseMatterSolver> matterSolver = std::make_unique<ConstDensityMatterSolver>(3, 2.6);
+ std::unique_ptr<BaseMatterSolver> matterSolver = 0 = std::make_unique<ConstDensityMatterSolver>(3, 2.6);
diff --git a/nuTens/propagator/const-density-solver.hpp b/nuTens/propagator/const-density-solver.hpp
index 5aa5b34..19470ea 100644
--- a/nuTens/propagator/const-density-solver.hpp
+++ b/nuTens/propagator/const-density-solver.hpp
@@ -82 +82 @@ 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 e3a587a..f825ea5 100644
--- a/nuTens/tensors/tensor.hpp
+++ b/nuTens/tensors/tensor.hpp
@@ -52 +52 @@ class Tensor
- Tensor(){};
+ Tensor()= default;;
@@ -56 +56 @@ 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 3324317..cf91698 100644
--- a/nuTens/tensors/torch-tensor.cpp
+++ b/nuTens/tensors/torch-tensor.cpp
@@ -20 +20 @@ 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)
Have any feedback or feature suggestions? Share it here.
|
|
||
| /// @brief Construct a 1-d array with specified values | ||
| /// @arg values The values to include in the tensor | ||
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, |
There was a problem hiding this comment.
clang-format suggestions
Suggested change
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, | |
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, |
| bool requiresGrad = true); | ||
| /// @brief Initialise this tensor with ones | ||
| /// @brief Default constructor with no initialisation | ||
| Tensor(){}; |
There was a problem hiding this comment.
clang-tidy diagnostics
- use '= default' to define a trivial default constructor [modernize-use-equals-default]
Suggested change
| Tensor(){}; | |
| Tensor()= default;; |
|
|
||
| /// @brief Construct a 1-d array with specified values | ||
| /// @arg values The values to include in the tensor | ||
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, |
There was a problem hiding this comment.
clang-tidy suggestions
Suggested change
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, | |
| Tensor(std::vector<float> values, NTdtypes::scalarType type = NTdtypes::kFloat, |
| @@ -17,45 +17,72 @@ std::string Tensor::getTensorLibrary() | |||
| return "PyTorch"; | |||
| } | |||
|
|
|||
| Tensor &Tensor::ones(int length, NTdtypes::scalarType type, NTdtypes::deviceType device, bool requiresGrad) | |||
| Tensor::Tensor(std::vector<float> values, NTdtypes::scalarType type, NTdtypes::deviceType device, bool requiresGrad) | |||
There was a problem hiding this comment.
clang-tidy diagnostics
- the parameter 'values' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
Suggested change
| 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) |
|
| Report | Thu, August 8, 2024 at 23:10:49 UTC |
| Project | nuTens |
| Branch | feature_improve_tensor_interface |
| Testbed | ubuntu-latest |
⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!
- Latency (latency)
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-thresholdsCLI flag.
Click to view all benchmark results
| Benchmark | Latency | Latency Results nanoseconds (ns) |
|---|---|---|
| Const Density Oscillations/1024/1024 | ➖ (view plot) | 4,689,771,572.00 |
| Const Density Oscillations/1024/1024_cv | ➖ (view plot) | 0.00 |
| Const Density Oscillations/1024/1024_mean | ➖ (view plot) | 4,686,469,794.37 |
| Const Density Oscillations/1024/1024_median | ➖ (view plot) | 4,678,853,041.00 |
| Const Density Oscillations/1024/1024_stddev | ➖ (view plot) | 17,161,362.70 |
| Vacuum Oscillations/1024/1024 | ➖ (view plot) | 262,678,301.33 |
| Vacuum Oscillations/1024/1024_cv | ➖ (view plot) | 0.04 |
| Vacuum Oscillations/1024/1024_mean | ➖ (view plot) | 267,452,796.25 |
| Vacuum Oscillations/1024/1024_median | ➖ (view plot) | 262,925,851.50 |
| Vacuum Oscillations/1024/1024_stddev | ➖ (view plot) | 10,305,432.72 |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Slightly tidier way if building tensor objects and some new constructors to simplify things a bit