changeset 1153:f1bb1b6d85dd refactor/sbpoperators/inflation

Review: Suggest changes to test and removal of conveninece constructor
author Vidar Stiernström <vidar.stiernstrom@it.uu.se>
date Tue, 25 Oct 2022 10:33:27 +0200
parents 56bc2c6a17fd
children ae006e844870
files src/SbpOperators/boundaryops/boundary_operator.jl test/LazyTensors/lazy_tensor_operations_test.jl test/SbpOperators/boundaryops/boundary_operator_test.jl test/SbpOperators/volumeops/derivatives/first_derivative_test.jl test/SbpOperators/volumeops/derivatives/second_derivative_test.jl test/SbpOperators/volumeops/volume_operator_test.jl
diffstat 6 files changed, 16 insertions(+), 2 deletions(-) [+]
line wrap: on
line diff
--- a/src/SbpOperators/boundaryops/boundary_operator.jl	Fri Oct 21 23:07:30 2022 +0200
+++ b/src/SbpOperators/boundaryops/boundary_operator.jl	Tue Oct 25 10:33:27 2022 +0200
@@ -14,6 +14,10 @@
 
 BoundaryOperator{R}(stencil::Stencil{T,N}, size::Int) where {T,R,N} = BoundaryOperator{T,R,N}(stencil, size)
 # TBD: Will the above convenience constructor ever be used?
+# Review: 
+# It should probably be removed. It's kind of weird to have a convenice constructor
+# for a general type if it isnt used by any of the current specializations. And in most cases the 1D constructor
+# can be inflated.
 
 """
     BoundaryOperator(grid::EquidistantGrid{1}, closure_stencil, region)
--- a/test/LazyTensors/lazy_tensor_operations_test.jl	Fri Oct 21 23:07:30 2022 +0200
+++ b/test/LazyTensors/lazy_tensor_operations_test.jl	Tue Oct 25 10:33:27 2022 +0200
@@ -367,6 +367,8 @@
     end
 end
 
+# Review: If we are thorough in testing multi-D tensors here we can drop tests from many of the volume operators,
+# e.g. first derivative and second derivative.
 @testset "inflate" begin
     I = LazyTensors.inflate(IdentityTensor(),(3,4,5,6), 2)
     @test I isa LazyTensor{Float64, 3,3}
--- a/test/SbpOperators/boundaryops/boundary_operator_test.jl	Fri Oct 21 23:07:30 2022 +0200
+++ b/test/SbpOperators/boundaryops/boundary_operator_test.jl	Tue Oct 25 10:33:27 2022 +0200
@@ -14,10 +14,11 @@
     g_2D = EquidistantGrid((11,15), (0.0, 0.0), (1.0,1.0))
 
     @testset "Constructors" begin
+        # Review: Remove the first line below in case we remove convenience constructor in BoundaryOperator
         op_l = BoundaryOperator{Lower}(closure_stencil,size(g_1D)[1])
         @test op_l == BoundaryOperator(g_1D,closure_stencil,Lower())
         @test op_l isa LazyTensor{T,0,1} where T
-
+         # Review: Remove the first line below in case we remove convenience constructor in BoundaryOperator
         op_r = BoundaryOperator{Upper}(closure_stencil,size(g_1D)[1])
         @test op_r == BoundaryOperator(g_1D,closure_stencil,Upper())
         @test op_r isa LazyTensor{T,0,1} where T
--- a/test/SbpOperators/volumeops/derivatives/first_derivative_test.jl	Fri Oct 21 23:07:30 2022 +0200
+++ b/test/SbpOperators/volumeops/derivatives/first_derivative_test.jl	Tue Oct 25 10:33:27 2022 +0200
@@ -19,7 +19,9 @@
     end
     x^k/factorial(k)
 end
-
+# Review:
+# If we test LazyTensor.inflation for e.g 1D-3D general tensors then we should only need to test
+# the 1D first derivative.
 @testset "first_derivative" begin
     @testset "Constructors" begin
         stencil_set = read_stencil_set(sbp_operators_path()*"standard_diagonal.toml"; order=2)
--- a/test/SbpOperators/volumeops/derivatives/second_derivative_test.jl	Fri Oct 21 23:07:30 2022 +0200
+++ b/test/SbpOperators/volumeops/derivatives/second_derivative_test.jl	Tue Oct 25 10:33:27 2022 +0200
@@ -6,6 +6,9 @@
 
 import Sbplib.SbpOperators.VolumeOperator
 
+# Review:
+# If we test LazyTensor.inflation for e.g 1D-3D general tensors then we should only need to test
+# the 1D second derivative.
 @testset "SecondDerivative" begin
     operator_path = sbp_operators_path()*"standard_diagonal.toml"
     stencil_set = read_stencil_set(operator_path; order=4)
--- a/test/SbpOperators/volumeops/volume_operator_test.jl	Fri Oct 21 23:07:30 2022 +0200
+++ b/test/SbpOperators/volumeops/volume_operator_test.jl	Tue Oct 25 10:33:27 2022 +0200
@@ -40,6 +40,8 @@
     r_even[1] = (v[1] + v[2])/2
     r_odd[1]  = (v[1] + v[2])/2
 
+    # Review:
+    # Use multiplication symbol, e.g. 2*v[1]
     r_even[2] = 2v[1] + v[2]
     r_odd[2]  = 2v[1] + v[2]