Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test TiledFactorization in CI #17

Merged
merged 1 commit into from
Jul 2, 2022
Merged

Conversation

ffevotte
Copy link
Collaborator

@ffevotte ffevotte commented Jul 1, 2022

With this PR, the CI workflow integrates new steps during which https://github.com/maltezfaria/TiledFactorization is cloned and its automated tests are passed. One advantage of integrating this as new steps in the existing workflow is that code coverage statistics should account for the code paths activated by TiledFactorization.

FWIW, an alternate way of doing things would have been to define a new, dedicated job (alongside the "Julia" test matrix and the "Documentation" job). This could make it easier to see whether failures come from internal tests or TiledFactorization, but it would not easily allow code coverage statistics to include TiledFactorization tests.

@ffevotte
Copy link
Collaborator Author

ffevotte commented Jul 1, 2022

To expand on the code coverage topic, it looks like TiledFactorization tests src/arrayinterface.jl a bit more extensively than the internal tests in DataFlowTasks:

https://app.codecov.io/gh/maltezfaria/DataFlowTasks.jl/commit/656d2a164cec9c990316c52f54232b2ae61df473

@ffevotte
Copy link
Collaborator Author

ffevotte commented Jul 1, 2022

I also wanted to say that TiledFactorization tests tend to take a very long time to pass (although they are accasionally shorter). I suspect compilation-induced latency to be the culprit here, but I haven't investigated much.

@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #17 (656d2a1) into main (50c54ab) will increase coverage by 0.32%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
+ Coverage   57.86%   58.18%   +0.32%     
==========================================
  Files           8        8              
  Lines         617      617              
==========================================
+ Hits          357      359       +2     
+ Misses        260      258       -2     
Impacted Files Coverage Δ
src/arrayinterface.jl 88.88% <0.00%> (+11.11%) ⬆️

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 50c54ab...656d2a1. Read the comment docs.

@maltezfaria
Copy link
Owner

To expand on the code coverage topic, it looks like TiledFactorization tests src/arrayinterface.jl a bit more extensively than the internal tests in DataFlowTasks:

https://app.codecov.io/gh/maltezfaria/DataFlowTasks.jl/commit/656d2a164cec9c990316c52f54232b2ae61df473

That makes sense as TiledFactorization makes extensive use of the Array interface. It is very nice to have the test and test coverage automated -- thanks!

@maltezfaria
Copy link
Owner

I also wanted to say that TiledFactorization tests tend to take a very long time to pass (although they are accasionally shorter). I suspect compilation-induced latency to be the culprit here, but I haven't investigated much.

I think you are correct -- the culprit is the compilation-induced latency. To be more precise, I think the issue comes from the use of @turbo from TiledFactorization to generate some efficient kernels for the small sized _chol and _lu methods.

Note: After seeing that most of the time is actually spent doing the Schur complement, which is delegated to Octavian, I wonder if we could not simply use the generic ldiv and chol methods for the LinearAlgebra. That would significantly simply the package, reduce the number of lines of code, and drop a few "heavy" dependencies. Perhaps worth testing?

@maltezfaria maltezfaria merged commit 443dc63 into main Jul 2, 2022
@maltezfaria maltezfaria mentioned this pull request Jul 2, 2022
2 tasks
@ffevotte ffevotte deleted the ff/ci-TiledFactorization branch August 24, 2022 21:21
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.

2 participants