Bugfix: testvalue for LinearAlgera.Diagonal

[JordiManyer]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 94.73684% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.84%. Comparing base (f39eac1) to head (cef3e53).
⚠️ Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Fields/DiffOperators.jl	66.66%	4 Missing ⚠️
src/Arrays/Interface.jl	50.00%	3 Missing ⚠️
src/Fields/FieldsInterfaces.jl	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1290      +/-   ##
==========================================
+ Coverage   88.83%   88.84%   +0.01%     
==========================================
  Files         227      227              
  Lines       29775    29799      +24     
==========================================
+ Hits        26451    26476      +25     
+ Misses       3324     3323       -1     

Flag	Coverage Δ
drivers	39.91% <72.36%> (-0.05%)	⬇️
extensions	5.07% <13.42%> (-0.06%)	⬇️
unit-adaptivity	40.05% <63.81%> (-0.06%)	⬇️
unit-basics	14.71% <47.36%> (+0.01%)	⬆️
unit-celldata	21.11% <65.13%> (-0.03%)	⬇️
unit-fespaces-1	32.34% <67.10%> (-0.03%)	⬇️
unit-fespaces-2	38.83% <68.42%> (-0.01%)	⬇️
unit-fields	17.72% <82.23%> (+0.14%)	⬆️
unit-geometry	28.75% <63.15%> (-0.03%)	⬇️
unit-multifield	30.84% <61.84%> (-0.04%)	⬇️
unit-odes	28.71% <55.26%> (-0.05%)	⬇️
unit-referencefes	34.36% <73.02%> (+0.10%)	⬆️
unit-visualization	11.84% <31.57%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JordiManyer]

@Antoinemarteau we have an issue... LinearAlgebra.Diagonal needs to be a square matrix, but the current return_value for LinearCombinationVector wants to generate a 0xn_basis, so we actually change the type of what we return. I added tests that currently fail.

This is quite horrible, and I do not see a way out other than removing the diagonals in favor of plain matrices, or implementing our own flavor of diagonal coefficients that allows for non-square matrices. What do you think?

[Antoinemarteau]

Or, change testvalue of CompWiseTensorPolyBasis (and all other bases) to return a zero length basis, like CartProdPolyBasis does ?
The only polynomial basis for which it is not possible to make it empty is NedelecPolyBasisOnSimplex, but it is quite unused ...

[Antoinemarteau]

Alternatively, testvalue of LinearCombinationFieldVector could simply be consistent with the length of the basis returned by testvalue(F)?
values = zeros(eltype(V), length(fields), 0)
becomes
values = Diagonal(zeros(eltype(V), length(fields), length(fields)))
if V <: Diagonal ?

[JordiManyer]

@Antoinemarteau Not only you are using Diagonal, you were using Diagonal of weird stuff, like MVectors and Ones. The worse part is that you can't even use similar on those, because they return sregular arrays. Its a mess... is it really worth it the optimisation? If you want to keep it like this, could you have a look on how to make it viable? I guess this will only appear for linear combinations, but the amount of possibilities is quite bad right now.

[Antoinemarteau]

It's probably not worth it to use MVectors at evaluation time, and might not change anything if the evaluation of LinearCombination[...] uses @inbounds, as the size can be determined from the field...
The downside would be allocations when building the reffes (only an issue if many are build).

[JordiManyer]

Also, unless you have MVectors everywhere I dont think there is any loop unrolling

[Antoinemarteau]

This still looks weird to me, V is ignored so the returned testvalue isn't necessary of the same type as the argument, same with the FieldVector version (if V isn't Diagonal). Doesn't this break the API of testvalue ?