Skip to content

Conversation

oriolcg
Copy link
Member

@oriolcg oriolcg commented Feb 28, 2022

Replacing fill! by fillstored in GridapODEs functions. This PR needs to be released for PR gridap/GridapDistributed.jl#81 to pass the tests.

@oriolcg
Copy link
Member Author

oriolcg commented Feb 28, 2022

Leaving fill! for dense matrices since fillstored! is not implemented for this case. Using only fillstored! when issparse(A) returns true.

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2022

Codecov Report

Merging #751 (a025028) into master (630f3c6) will increase coverage by 0.02%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #751      +/-   ##
==========================================
+ Coverage   88.32%   88.35%   +0.02%     
==========================================
  Files         152      152              
  Lines       16595    16595              
==========================================
+ Hits        14658    14662       +4     
+ Misses       1937     1933       -4     
Impacted Files Coverage Δ
src/GridapODEs/ODETools/ODETools.jl 100.00% <ø> (ø)
src/GridapODEs/ODETools/AffineThetaMethod.jl 83.52% <66.66%> (ø)
src/GridapODEs/ODETools/AffineNewmark.jl 100.00% <100.00%> (ø)
src/GridapODEs/ODETools/ConstantMatrixNewmark.jl 87.50% <100.00%> (ø)
src/GridapODEs/ODETools/ConstantNewmark.jl 96.20% <100.00%> (ø)
src/GridapODEs/ODETools/ForwardEuler.jl 87.87% <100.00%> (ø)
src/GridapODEs/ODETools/Newmark.jl 91.66% <100.00%> (ø)
src/GridapODEs/ODETools/RungeKutta.jl 94.01% <100.00%> (ø)
src/GridapODEs/ODETools/ThetaMethod.jl 97.72% <100.00%> (ø)
src/TensorValues/Operations.jl 90.23% <0.00%> (+1.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 630f3c6...a025028. Read the comment docs.

@santiagobadia
Copy link
Member

santiagobadia commented Mar 1, 2022

@oriolcg can we create a method that does the dispatch for each case?
I think it would be better than replicating the if statements in so many places
E.g., check whether the matrix is sparse or not inside fill_entries! (pick name) and call internally fill! if dense or fillstore! if sparse. Does it make sense?

@santiagobadia
Copy link
Member

The other question is, do we really need to run the code for dense matrices?

If not, I would use fillstored! everywhere

@oriolcg
Copy link
Member Author

oriolcg commented Mar 2, 2022

The other question is, do we really need to run the code for dense matrices?

If not, I would use fillstored! everywhere

We were using dense matrices in the tests. I defined sparse matrix in ODEOperatorMocks and left fillstored! only in a025028.

@santiagobadia
Copy link
Member

Ok much better. Thanks!

@oriolcg
Copy link
Member Author

oriolcg commented Mar 3, 2022

Hi @santiagobadia , this PR has been closed without being merged.

@oriolcg oriolcg reopened this Mar 3, 2022
@santiagobadia santiagobadia merged commit 7af5578 into master Mar 4, 2022
@santiagobadia santiagobadia deleted the fill_to_fillstored_GridapODEs branch March 4, 2022 10:30
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.

3 participants