SpecialMatrices.jl icon indicating copy to clipboard operation
SpecialMatrices.jl copied to clipboard

Clean and 100% covered Cauchy matrices

Open fp4code opened this issue 8 years ago • 11 comments

  • .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)

fp4code avatar Feb 16 '17 19:02 fp4code

Coverage Status

Coverage increased (+5.6%) to 50.562% when pulling 3e21f6c937dc97ddeee1e0ec2c3cd5c188a6723c on fp4code:fp-cauchy-pr into babf5fb579a73be9048405dd87267866a38eda5c on jiahao:master.

coveralls avatar Feb 16 '17 19:02 coveralls

@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?

JeffFessler avatar Aug 31 '21 01:08 JeffFessler

@fp4code can you make the setting for the PR to "allow edits by maintainers"? If not then I'll have to start from scratch...

JeffFessler avatar Oct 25 '22 17:10 JeffFessler

Yes of course, @JeffFessler. I'm looking into how to "allow edits by maintainers"!

fp4code avatar Oct 26 '22 08:10 fp4code

@JeffFessler, it looks like the red box on the right was already checked. Please, let me know.

fp4code avatar Oct 26 '22 08:10 fp4code

@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.

JeffFessler avatar Oct 26 '22 11:10 JeffFessler

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.

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 Coverage Status
Change from base Build 3321757331: 4.3%
Covered Lines: 218
Relevant Lines: 225

💛 - Coveralls

coveralls avatar Oct 26 '22 15:10 coveralls

Ok @fp4code this took me too many tries, but finally I got it to pass tests with type inference. Could you please review?

JeffFessler avatar Oct 26 '22 15:10 JeffFessler

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.

codecov[bot] avatar Nov 03 '22 23:11 codecov[bot]

@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.

JeffFessler avatar Nov 05 '22 15:11 JeffFessler

Hi @fp4code and @mkitti, could you double check this updated PR?

JeffFessler avatar Nov 10 '22 17:11 JeffFessler

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.

JeffFessler avatar Jan 01 '23 17:01 JeffFessler