Skip to content

Conversation

@nantonel
Copy link
Member

@nantonel nantonel commented Sep 7, 2018

Making compatible with Julia 0.7 and 1.0

@nantonel nantonel requested a review from lostella September 10, 2018 11:02
Copy link
Member

@lostella lostella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should simplify how adjoint operators are handled

# build mul!(y, H.A[1], b[H.idxs[1]])
bb = :(b[H.idxs[1]])
else
# staked operator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stacked

Copy link
Member

@lostella lostella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor comments/request. Congratulations, this must have required some patience!

return (domain...,)
end
codomainType(L::HCAT) = codomainType.(L.A[1])
codomainType(L::HCAT) = codomainType.(Ref(L.A[1]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this Ref? I'm just curious, I don't now how that works

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the 0.7 release notes

Previously, broadcast defaulted to treating its arguments as scalars if they were not arrays. This behavior is deprecated, and in the future broadcast will default to iterating over all its arguments. Wrap arguments you wish to be treated as scalars with Ref() or a 1-tuple. Package developers can choose to allow a non-iterable type T to always behave as a scalar by implementing broadcastable(x::T) = Ref(x) (#26212).

I'm still digesting this though...


struct LMatrixOp{T, A <: Union{AbstractVector,AbstractMatrix},
B <:Union{RowVector,AbstractMatrix}} <: LinearOperator
B <:Union{Adjoint,AbstractMatrix}} <: LinearOperator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it: Adjoint <: AbstractMatrix, so this Union seems redundant.

@lostella
Copy link
Member

lostella commented Sep 11, 2018

Another request:

  • update README.md!

@codecov-io
Copy link

codecov-io commented Sep 12, 2018

Codecov Report

Merging #4 into master will increase coverage by 4.73%.
The diff coverage is 95.29%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #4      +/-   ##
========================================
+ Coverage   92.26%    97%   +4.73%     
========================================
  Files          44     44              
  Lines        1564   1201     -363     
========================================
- Hits         1443   1165     -278     
+ Misses        121     36      -85
Impacted Files Coverage Δ
src/calculus/Sum.jl 100% <100%> (+5.45%) ⬆️
src/linearoperators/LBFGS.jl 100% <100%> (ø) ⬆️
src/calculus/Reshape.jl 100% <100%> (ø) ⬆️
src/linearoperators/FiniteDiff.jl 100% <100%> (ø) ⬆️
src/utilities/block.jl 100% <100%> (+11.76%) ⬆️
src/nonlinearoperators/Pow.jl 100% <100%> (ø) ⬆️
src/linearoperators/DFT.jl 100% <100%> (ø) ⬆️
src/calculus/AdjointOperator.jl 100% <100%> (ø)
src/nonlinearoperators/Tanh.jl 100% <100%> (ø) ⬆️
src/linearoperators/LMatrixOp.jl 100% <100%> (ø)
... and 76 more

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 632f179...0f796da. Read the comment docs.

@nantonel nantonel merged commit 1e5a7a9 into kul-optec:master Sep 12, 2018
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.

3 participants