Clean and 100% covered Cauchy matrices #17

fp4code · 2017-02-16T19:33:30Z

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

coveralls · 2017-02-16T19:44:51Z

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

jiahao

Thanks for the PR and apologies for being MIA for so long!

This PR certainly improve the current state of the code. I would accept it after the size(A, n) method is restored (it is needed separately from size(A) as part of the array interface).

src/cauchy.jl

JeffFessler · 2021-08-31T01:50:24Z

@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 · 2022-10-25T17:34:35Z

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

fp4code · 2022-10-26T08:01:07Z

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

fp4code · 2022-10-26T08:04:39Z

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

These days we use github actions not travis.

Because `Matrix(A)` is the way to do that now.

Refine tests

size(A,i) no longer needed

JeffFessler · 2022-10-26T11:20:10Z

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

hopefully...

coveralls · 2022-10-26T15:49:14Z

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

JeffFessler · 2022-10-26T15:57:21Z

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

src/cauchy.jl

mkitti · 2022-11-03T23:03:04Z

src/cauchy.jl

+        Tx = eltype(first(x))
+        Ty = eltype(first(y))
+        all(==(Tx), eltype.(x)) || throw(ArgumentError("inconsistent x element types"))
+        all(==(Ty), eltype.(y)) || throw(ArgumentError("inconsistent y element types"))


Are x and y more abstract than an AbstractArray{T}? Could we enforce consistency in another way?

AFAIK, in every other special matrix in this package, the input types (when relevant) were originally just Vector and we have generalized many of them to AbstractVector so that users can use ranges like 1:3.
For reasons unknown to me, this particular special matrix allowed more general iterable inputs, like a tuple of the form (1.0, 1//2, 3f0). The test case even had a row vector of the form [2im 3f0] or such. Honestly I don't quite know what the use case is for supporting more general iterables. However, the wiki page defines x_i and y_i to be two "injective sequences" so that language is more general than vectors. Perhaps the original code authors were intending hold true to that definition.

The "injective" part of that definition means that each of these two sequences should have unique elements, and currently the code was not testing for that. I will add a test that reports a warning - let me know what you think.

Actually, I've looked back at the history of the original PR by @fp4code
https://github.com/fp4code/SpecialMatrices.jl/blob/e5036efc4cc3c6ece11be6e9c0d90e5b0a9019ae/test/cauchy.jl
and it looks like the most general type being tested was AbstractArray, not more general iterables.
So it would be fine with me to simplify this constructor to take only AbstractArray types with 1-based indexing.
Would that be preferable to you @mkitti ?

I think we should definitely write a method for AbstractVector{T}. We can even leave the older implementation here.

mkitti · 2022-11-03T23:05:00Z

src/cauchy.jl

+        Ty = eltype(first(y))
+        all(==(Tx), eltype.(x)) || throw(ArgumentError("inconsistent x element types"))
+        all(==(Ty), eltype.(y)) || throw(ArgumentError("inconsistent y element types"))
+        T = eltype(one(Tx) + one(Ty)) 


Could we use the type promotion system here directly?

Originally I had code that used something like promote_type(x...) but it failed @inferred for some tests. Apparently the compiler was unable to do type inference when the input x is an iterable with a mix of several element types.
So I resorted to this all approach to make type inference work.
I'm open to suggestions. (Ideally Julia would have an abstract type for an iterable where all elements are the same type, and AbstractArray would be a subtype of that parent type.)

Co-authored-by: Mark Kittisopikul <[email protected]>

codecov · 2022-11-03T23:58:43Z

Codecov Report

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

JeffFessler · 2022-11-05T15:29:26Z

@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 · 2022-11-10T17:56:51Z

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

JeffFessler · 2023-01-01T17:41:59Z

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.

fp4code added 6 commits February 16, 2017 17:25

Modernize Travis configuration

d9983d8

Temporary rebadge

1d80827

Clean and add tests for Cauchy matrices

e5036ef

Add Cauchy test to runtests.jl

fa929c3

Use collect for better julia-0.4 compatibility

724642c

Rebadge for pull-request

3e21f6c

fp4code changed the title ~~Clean and 100% covered Hankel matrices~~ Clean and 100% covered Cauchy matrices Feb 16, 2017

jiahao previously requested changes Aug 22, 2017

View reviewed changes

src/cauchy.jl Outdated Show resolved Hide resolved

JeffFessler mentioned this pull request Aug 13, 2021

Cauchy type mismatch #46

Closed

JeffFessler added 4 commits October 26, 2022 06:54

Delete .travis.yml

8ab1de6

These days we use github actions not travis.

Add docstring

a7e0fcc

Remove "full"

1b96b10

Because `Matrix(A)` is the way to do that now.

Update cauchy.jl

9af7be8

Refine tests

Merge branch 'master' into fp-cauchy-pr

d2193fb

JeffFessler added 5 commits October 26, 2022 07:27

Handle 1:k using Rational

0e76d2e

fix test

56de4f7

fix type promotion

18e0335

hopefully...

Args must be vectors

231e398

test with vec

df8f899

JeffFessler marked this pull request as draft October 26, 2022 13:36

JeffFessler added 2 commits October 26, 2022 11:46

Handle general iterables with proper type

c93c288

Update tests

b369d28

JeffFessler marked this pull request as ready for review October 26, 2022 15:56

JeffFessler requested a review from ivanslapnicar October 26, 2022 20:45

JeffFessler mentioned this pull request Nov 3, 2022

Use 3-arg mul! for Companion and improve test coverage #69

Merged

mkitti reviewed Nov 3, 2022

View reviewed changes

src/cauchy.jl Outdated Show resolved Hide resolved

mkitti reviewed Nov 3, 2022

View reviewed changes

mkitti suggested changes Nov 3, 2022

View reviewed changes

Use jldoctest

2c3d16a

Co-authored-by: Mark Kittisopikul <[email protected]>

JeffFessler added 4 commits November 3, 2022 20:15

Check that x,y elements are unique

a0e3ae0

Standardize docstring

d5d81df

Specialize constructor for AbstractVector

cda64dc

Limit to NTuple and AbstractArray

80e4367

JeffFessler added 2 commits November 5, 2022 11:31

Fix comment about iterables

afd6eaf

Fix typo

1e6c851

JeffFessler requested review from jishnub and removed request for ivanslapnicar November 15, 2022 17:32

JeffFessler mentioned this pull request Dec 27, 2022

Overload Base.map #74

Open

Use typeof not eltype, and Type not DataType

00e7172

JeffFessler merged commit aa1bd90 into JuliaLinearAlgebra:master Jan 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean and 100% covered Cauchy matrices #17

Clean and 100% covered Cauchy matrices #17

fp4code commented Feb 16, 2017 •

edited by JeffFessler

Loading

coveralls commented Feb 16, 2017 •

edited

Loading

jiahao left a comment

JeffFessler commented Aug 31, 2021

JeffFessler commented Oct 25, 2022

fp4code commented Oct 26, 2022

fp4code commented Oct 26, 2022

JeffFessler commented Oct 26, 2022

coveralls commented Oct 26, 2022 •

edited

Loading

JeffFessler commented Oct 26, 2022

mkitti Nov 3, 2022

JeffFessler Nov 4, 2022

JeffFessler Nov 4, 2022

mkitti Nov 4, 2022

mkitti Nov 3, 2022

JeffFessler Nov 4, 2022

codecov bot commented Nov 3, 2022 •

edited

Loading

JeffFessler commented Nov 5, 2022

JeffFessler commented Nov 10, 2022

JeffFessler commented Jan 1, 2023

Clean and 100% covered Cauchy matrices #17

Clean and 100% covered Cauchy matrices #17

Conversation

fp4code commented Feb 16, 2017 • edited by JeffFessler Loading

coveralls commented Feb 16, 2017 • edited Loading

jiahao left a comment

Choose a reason for hiding this comment

JeffFessler commented Aug 31, 2021

JeffFessler commented Oct 25, 2022

fp4code commented Oct 26, 2022

fp4code commented Oct 26, 2022

JeffFessler commented Oct 26, 2022

coveralls commented Oct 26, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3330509665

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

JeffFessler commented Oct 26, 2022

mkitti Nov 3, 2022

Choose a reason for hiding this comment

JeffFessler Nov 4, 2022

Choose a reason for hiding this comment

JeffFessler Nov 4, 2022

Choose a reason for hiding this comment

mkitti Nov 4, 2022

Choose a reason for hiding this comment

mkitti Nov 3, 2022

Choose a reason for hiding this comment

JeffFessler Nov 4, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 3, 2022 • edited Loading

Codecov Report

JeffFessler commented Nov 5, 2022

JeffFessler commented Nov 10, 2022

JeffFessler commented Jan 1, 2023

fp4code commented Feb 16, 2017 •

edited by JeffFessler

Loading

coveralls commented Feb 16, 2017 •

edited

Loading

coveralls commented Oct 26, 2022 •

edited

Loading

codecov bot commented Nov 3, 2022 •

edited

Loading