Adding internals for array functions #8
Conversation
In the future I would overload _get_coefficients or the constructor Weeks to handle array valued Laplace functions.
Note: I'm unsure if the Weeks(func, ...) constructor should contain a parameter describing the dimensions of func. Right now I'll have to rewrite _get_coefficients when func gives an array.
Now I noticed that the 2x2 test case has been transposed, giving false test passes. The isapprox(laguerreeval,...) test passes if the matrix [1 3; 2 4] is used, which is wrong.
Permutedims!(...) was supposed to only shift the last index to the first place. Previously, it incorrectly reversed the ordering of the indices (essentially giving a transposed FFT in the end). The unit tests missed this since they were also written in a row-major order. The tests now correctly test in a memory efficient way (column-major order) and some have been reduced to a single line.
Although this functionality is not in use, I noticed that
_get_array_coefficients(w::Weeks{T}) would use the scalar method if
called.
This has now been fixed.
To keep things simple for now, I'll ignore
_set_array_coefficients(w::Weeks{T}).
eval_ilt(w::Weeks) works for scalar functions. It currently does not work for arrays, since the dimensionality of w.coefficients is unknown. To test the ILT functionality for arrays, I'll calculate the coefficients directly and compare that with the scalar Weeks method.
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
===========================================
- Coverage 100% 75.67% -24.33%
===========================================
Files 5 5
Lines 115 185 +70
===========================================
+ Hits 115 140 +25
- Misses 0 45 +45
Continue to review full report at Codecov.
|
|
Thanks! I will look at the changes. The test coverage went down even thought you added tests. Any idea why ? |
|
Not really. Does this have something to do with the missing base report? |
test/arrayweeks_test.jl
Outdated
| let arr = try InverseLaplace._arrcoeff(arrFcomplex,4,1.0,1.0); | ||
| InverseLaplace._arrcoeff(arrFcomplex,4,1.0,1.0) | ||
| catch | ||
| InverseLaplace._arrcoeff(arrFcomplex,4,1.0,1.0) |
There was a problem hiding this comment.
Is it necessary to test _arrcoeff if _get_array_coefficients is tested as well?
There was a problem hiding this comment.
No, it shouldn't be necessary. I don't know whats up. 100 % coverage before is suspicious. Still, it may be accurate.
|
Yeah, maybe the missing base report. |
|
Others seem to have similar issues with coverage. |
The coefficient-rank in the Weeks{T}-type is no longer declared
explicitly. Instead, it is supplied as an argument to the outer
constructor.
Array- and complex-tests have been reformatted and grouped into
testsets.
elisno
force-pushed
the
laguerreoverload
branch
from
1c19498 to
bd27190
Compare
| end | ||
|
|
||
| @testset "Array functionality" begin | ||
| @testset "Internal functions" begin |
There was a problem hiding this comment.
This testset "Internal functions" won't be necessary after the commit elisno@bd27190,
where the Weeks-type can is finally implemented for arrays.
jlapeyre
force-pushed
the
master
branch
3 times, most recently
from
c95f98b to
5e6f821
Compare
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
============================================
- Coverage 100.00% 75.67% -24.33%
============================================
Files 5 5
Lines 115 185 +70
============================================
+ Hits 115 140 +25
- Misses 0 45 +45
Continue to review full report at Codecov.
|
4 participants
My previous pull request broke existing functionality, so I decided to restart my work and add internal functions for arrays.
If the function is an N-dimensional array, its Laguerre-coefficients are stored in an (N+1)-dimensional array. The FFT is performed along the first dimension of that coefficient array when calculating the Laguerre-series.
In the test file specific to array-valued functions (
test/arrayweeks_test.jl), calls to the internal functions seem to require two attempts (the first one usually outputs arrays of zeros). I haven't bothered to work on that, but I'll create a separate issue for that soon.