Streamline Vandermonde #49
Conversation
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
- Coverage 76.85% 74.22% -2.63%
==========================================
Files 8 8
Lines 216 194 -22
==========================================
- Hits 166 144 -22
Misses 50 50
Continue to review full report at Codecov.
|
|
@christopher-dG since you were one of the last editors of vandermonde.jl would you mind reviewing this PR? |
|
@devmotion you made recent significant contributions here - would you be able to take a look? |
There was a problem hiding this comment.
I didn't see any major problems. I added some comments and suggestions. In general, I'd suggest removing the commented code and at most keep comments for the remaining implementations. IMO the code is cleaner this way and the previous implementation can always be recovered from the git history.
| using SpecialMatrices: Vandermonde | ||
| import SpecialMatrices # dvand!, pvand! | ||
| using Test: @test, @testset, @test_throws, @inferred | ||
|
|
There was a problem hiding this comment.
These are not needed since runtests.jl loads SpecialMatrices and Test, and everything is exported.
| using SpecialMatrices: Vandermonde | |
| import SpecialMatrices # dvand!, pvand! | |
| using Test: @test, @testset, @test_throws, @inferred |
There was a problem hiding this comment.
Yes I know but I like to be able to just include("vandermonde.jl") when developing and have it be a self-contained test. And I feel that Julia code is more readable when you see the dependencies given explicitly at the top of each file. I agree it is not needed, but no real drawback, right?
There was a problem hiding this comment.
No, it's not completely uncommon. It's mainly surprising that all imported macros etc. are listed explicitly instead of
| using SpecialMatrices: Vandermonde | |
| import SpecialMatrices # dvand!, pvand! | |
| using Test: @test, @testset, @test_throws, @inferred | |
| using SpecialMatrices | |
| using Test |
as in runtests.jl. I assumed the intention was to not pollute the namespace which does not work since runtests.jl loads SpecialMatrices and Test.
There was a problem hiding this comment.
Yes I know - I'll work my way up to runtests.jl eventually.
After literally decades of using Matlab with no namespace control, perhaps I am overly excited about the opportunity to be very explicit 😃
src/vandermonde.jl
Outdated
| c :: C | ||
|
|
||
| function Vandermonde(c::AbstractVector{T}) where T | ||
| axes(c,1) isa Base.OneTo || throw(ArgumentError("must be OneTo")) |
There was a problem hiding this comment.
It would be nice to remove this restriction at some point.
There was a problem hiding this comment.
Thanks for all these helpful suggestions. I've accepted (almost) all of them.
But I am unsure what one would do with an OffsetVector input here.
I got into the habit of checking axes after working with @dlfivefifty on some other packages. So just now I checked to see what ToeplitzMatrices does with AbstractVector inputs:
At first it looked to me like it had no axes checking, but then I realized that the check is buried inside the convert call there. So if the vector is not something like OneTo then it throws a DimensionMismatch.
I think I will adopt the convert approach of ToeplitzMatrices and if y'all later figure out what would be a meaningful way to handle offsets then perhaps we can do that here too - if there is any use case!
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
Pull Request Test Coverage Report for Build 1184082679
💛 - Coveralls |
This PR supports construction with
AbstractVectorarguments:Vandermonde(1:3)instead of having to usecollect.It also provides more detailed docstring including
jldoctests.It eliminates some (I believe) unnecessary code. For now I have retained the code but enclosed it in block comments in case I have overlooked some reason to retain it.