SpecialMatrices.jl
SpecialMatrices.jl copied to clipboard
Clean and 100% covered Cauchy matrices
- .travis.yml is modernized
- test/cauchy.jl permits full coverage: https://coveralls.io/github/fp4code/SpecialMatrices.jl?branch=fp-cauchy
- src/cauchy.jl is cleaned
JF updates:
- not using travis anymore
- handle general NTuple and AbstractArray without
collect
- handle type carefully because matrix elements are reciprocals of the types of the "vector" inputs (this is especially important if the inputs have units; in that case the type of the matrix elements is definitely not the type of the vectors)
Coverage increased (+5.6%) to 50.562% when pulling 3e21f6c937dc97ddeee1e0ec2c3cd5c188a6723c on fp4code:fp-cauchy-pr into babf5fb579a73be9048405dd87267866a38eda5c on jiahao:master.
@fp4code this PR has some good work in it but the repo has changed greatly since the original PR. If you look at recent PRs you will see I am going through the types one-by-one and trying to clean them up akin to what you did here. Do you want to update this PR?
@fp4code can you make the setting for the PR to "allow edits by maintainers"? If not then I'll have to start from scratch...
Yes of course, @JeffFessler. I'm looking into how to "allow edits by maintainers"!
@JeffFessler, it looks like the red box on the right was already checked. Please, let me know.
@fp4code thanks, I figured out how to make updates. Lots of things changed in the 5 years (!) since your original PR. I made several updates in the web portal and it will probably take a few iterations for the tests to pass.
Pull Request Test Coverage Report for Build 3330509665
Warning: This coverage report may be inaccurate.
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+4.3%) to 96.889%
Totals | |
---|---|
Change from base Build 3321757331: | 4.3% |
Covered Lines: | 218 |
Relevant Lines: | 225 |
💛 - Coveralls
Ok @fp4code this took me too many tries, but finally I got it to pass tests with type inference. Could you please review?
Codecov Report
:exclamation: No coverage uploaded for pull request base (
master@a7b36cf
). Click here to learn what that means. Patch has no changes to coverable lines.
Additional details and impacted files
@@ Coverage Diff @@
## master #17 +/- ##
=========================================
Coverage ? 98.59%
=========================================
Files ? 8
Lines ? 213
Branches ? 0
=========================================
Hits ? 210
Misses ? 3
Partials ? 0
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
@mkitti there was a flaw in my previous implementation for iterables. The getindex
method requires 1-based indexing, and general iterables do not have such indexing. AFAIK the only viable arguments here are AbstractArray
with 1-based indexing, and NTuple
, so I have restructured the constructor to support "only" those two cases. This is more general than other lazy matrix types in this package (which used to support only Vectors and now also support AbstractVectors.) This set of arguments suffices to cover all the test cases that were there previously. It is ready for your next review.
Hi @fp4code and @mkitti, could you double check this updated PR?
Hi everyone, the bug in #46 has been sitting there for years and this PR will address it. My preference is to have a separate set of eyes on any code that is merged, so I've asked multiple people for reviews. But I've gotten no response, so finally I am going to go ahead and merge this. We can always improve it later if I've overlooked something.