fix mul! for JOLI operator and judiVector #114

ziyiyin97 · 2022-05-03T20:31:25Z

This PR makes

lsqr!(x, J*Mr, dobs)

work. Especially this works when Mr takes complex input (e.g. debiasing for curvelet)

codecov · 2022-05-03T20:36:08Z

Codecov Report

Merging #114 (867f9d4) into master (850f4b5) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

❗ Current head 867f9d4 differs from pull request most recent head 7f2b6c3. Consider uploading reports for the commit 7f2b6c3 to get more accurate results

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
- Coverage   82.42%   82.39%   -0.04%     
==========================================
  Files          24       24              
  Lines        1861     1863       +2     
==========================================
+ Hits         1534     1535       +1     
- Misses        327      328       +1

Impacted Files	Coverage Δ
src/TimeModeling/LinearOperators/operators.jl	`81.57% <33.33%> (-1.46%)`	⬇️
src/TimeModeling/Types/abstract.jl	`77.96% <66.66%> (ø)`
src/TimeModeling/LinearOperators/basics.jl	`77.27% <0.00%> (+2.27%)`	⬆️

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 850f4b5...7f2b6c3. Read the comment docs.

mloubout

I get what the aim is but it's not done correctly

mloubout · 2022-05-03T20:51:18Z

src/TimeModeling/LinearOperators/operators.jl

@@ -174,7 +174,7 @@ adjoint(L::LazyScal) = LazyScal(L.s, adjoint(L.P))

 mul!(out::SourceType{T}, F::judiPropagator{T, O}, q::SourceType{T}) where {T<:Number, O} = begin y = F*q; copyto!(out, y) end
 mul!(out::SourceType{T}, F::judiPropagator{T, O}, q::Vector{T}) where {T<:Number, O} = begin y = F*q; copyto!(out, y) end
-mul!(out::SourceType{T}, F::joLinearFunction{T, T}, q::SourceType{T}) where {T<:Number} = begin y = F*q; copyto!(out, y) end
+mul!(out::Union{SourceType{T1},SourceType{T2}}, F::joAbstractLinearOperator{T2, T1}, q::SourceType{T2}) where {T1<:Number, T2<:Number} = begin y = F*q; copyto!(out, y) end


No this is wrong has to be T1 to match F

Seems there is a problem in IterativeSolvers.jl JuliaLinearAlgebra/IterativeSolvers.jl#313 correct me if I am wrong

mloubout · 2022-05-03T20:51:39Z

src/TimeModeling/Types/abstract.jl

-(msv::judiMultiSourceVector{T})(x::Vector{T}) where {T<:AbstractFloat} = x
-(msv::judiMultiSourceVector{T})(x::judiMultiSourceVector{T}) where {T} = x
-(msv::judiMultiSourceVector{mT})(x::Vector{T}) where {mT, T<:Array} = begin y = deepcopy(msv); y.data .= x; return y end
+(msv::judiMultiSourceVector{mT})(x::AbstractVector{T}) where {mT<:Number, T<:Number} = x


No have to match and you are removing a case

judiMultiSourceVector{T} is AbstractVector so it includes the both cases already

The type has to match mT!=T is not valid.

mloubout · 2022-05-03T20:51:53Z

src/TimeModeling/Types/abstract.jl

-(msv::judiMultiSourceVector{T})(x::judiMultiSourceVector{T}) where {T} = x
-(msv::judiMultiSourceVector{mT})(x::Vector{T}) where {mT, T<:Array} = begin y = deepcopy(msv); y.data .= x; return y end
+(msv::judiMultiSourceVector{mT})(x::AbstractVector{T}) where {mT<:Number, T<:Number} = x
+(msv::judiMultiSourceVector{mT})(x::AbstractVector{T}) where {mT, T<:Array} = begin y = deepcopy(msv); y.data .= x; return y end


mloubout · 2022-05-03T21:20:19Z

examples/scripts/modeling_basic_2D.jl

@@ -172,6 +172,20 @@ ylabel("Depth (m)")
 title("RTM image")
 display(fig)

+#' notice that we can conduct a simplistic least-squares reverse-time migration via LSQR thanks to the linear algebraic abstraction


Completely wrong file please make an effort

mloubout · 2022-05-03T21:24:10Z

src/TimeModeling/LinearOperators/operators.jl

@@ -174,7 +174,8 @@ adjoint(L::LazyScal) = LazyScal(L.s, adjoint(L.P))

 mul!(out::SourceType{T}, F::judiPropagator{T, O}, q::SourceType{T}) where {T<:Number, O} = begin y = F*q; copyto!(out, y) end
 mul!(out::SourceType{T}, F::judiPropagator{T, O}, q::Vector{T}) where {T<:Number, O} = begin y = F*q; copyto!(out, y) end
-mul!(out::SourceType{T}, F::joLinearFunction{T, T}, q::SourceType{T}) where {T<:Number} = begin y = F*q; copyto!(out, y) end
+mul!(out::Union{SourceType{T1},SourceType{T2}}, F::joAbstractLinearOperator{T2, T1}, q::SourceType{T2}) where {T1<:Number, T2<:Number} = begin y = F*q; copyto!(out, y) end
+mul!(out::Union{SourceType{T1},SourceType{T2}}, F::joAbstractLinearOperator{T2, T1}, q::Array{T2, N}) where {T1<:Number, T2<:Number, N} = begin y = F*q[:]; copyto!(out, y) end


JoAbstractLinearOperator is too general

What's the potential problem of being too general?

It should at least contain the joLinearOperator and joLinearFunction

test/test_linear_algebra.jl

mloubout · 2022-05-31T20:00:43Z

examples/scripts/splsrtm_2D.jl

-title("LS-RTM with SGD"); xlabel("Lateral position [km]"); ylabel("Depth [km]")
+title("SPLS-RTM with SGD"); xlabel("Lateral position [km]"); ylabel("Depth [km]")
+
+#' LSQR


Can you please try to make a proper example. It's supposed to help new users so hiding this at the bottom of a file about sparsity isn't very friendly.

mloubout · 2022-06-24T11:28:02Z

examples/scripts/lsrtm_2D.jl

+niter = parse(Int, get(ENV, "NITER", "10"))
+lsqr_sol = zeros(Float32, prod(n))
+Ml = judiMarineTopmute2D(30, d_lin[1].geometry)
+lsqr!(lsqr_sol, Ml*J[1]*Mr, Ml*d_lin[1]; maxiter=niter)     # only invert the first shot record


Why only the first?

otherwise too long time in CI

This is an example, not a test you can't hardcode stuff like that in it. Like if anyone runs that the results is gonna look like JUDI produces shit.

mloubout · 2022-06-24T13:33:11Z

src/TimeModeling/Types/abstract.jl

-(msv::judiMultiSourceVector{T})(x::Vector{T}) where {T<:AbstractFloat} = x
-(msv::judiMultiSourceVector{T})(x::judiMultiSourceVector{T}) where {T} = x
-(msv::judiMultiSourceVector{mT})(x::Vector{T}) where {mT, T<:Array} = begin y = deepcopy(msv); y.data .= x; return y end
+(msv::judiMultiSourceVector{mT})(x::AbstractVector{T}) where {mT<:Number, T<:Number} = x


The type has to match mT!=T is not valid.

ziyiyin97 · 2022-06-24T16:26:07Z

Without (msv::judiMultiSourceVector{mT})(x::AbstractVector{T}) where {mT<:Number, T<:Number} = x then

JUDI.jl/test/test_linear_algebra.jl

Line 101 in e22998d

dm1 = adjoint(J*DFT') * dlin

is not passing. I do think we need to add this because again x and msv might live in different space.

mloubout · 2022-06-27T22:41:35Z

. I do think we need to add this because again x and msv might live in different space.

Then need to be added but properly:

add domain range in mul! and split back into Vector and judiMultiSource because judiMultiSource op cannot change data type unless they return a dense vector

ziyiyin97 · 2022-07-11T15:02:25Z

any other comment on this one?

mloubout · 2022-07-11T15:07:34Z

examples/scripts/lsrtm_2D.jl

+niter = parse(Int, get(ENV, "NITER", "10"))
+lsqr_sol = zeros(Float32, prod(n))
+
+# only invert for the randomly picked indices so that this example can run a bit faster


You are still hardoding a "want CI to run fast" into a user example. The default new user who runs it should get a decent result and the example produce a result, not "just run".

ziyiyin97 · 2022-07-11T15:29:15Z

Hmm the memory blows up in the CI test https://github.com/slimgroup/JUDI.jl/runs/7285219825?check_suite_focus=true any suggestion?

ziyiyin97 · 2022-07-11T15:51:06Z

Correct me if wrong but this parallelization seems to be the culprit

JUDI.jl/src/TimeModeling/Modeling/distributed.jl

Line 54 in 01eedad

    
           @async remotecall_wait(local_reduce!, futures[i].where, futures[i], futures[i+1])

because in principal it shouldn't insist to run the born/migration with all sources if not enough memory

mloubout · 2022-07-12T12:18:04Z

Correct me if wrong but this parallelization seems to be the culprit

I have no idea why this would be related to it this just reduces results pair by pairs.

mloubout · 2022-07-12T12:44:37Z

Also once again, please learn to rebase your branches I cannot do it for you every time.

ziyiyin97 force-pushed the fixabstractvec branch from 32ee9f1 to 9cdf930 Compare May 3, 2022 21:14

mloubout reviewed May 3, 2022

View reviewed changes

ziyiyin97 changed the title ~~fix mul! add associated test~~ fix mul! for JOLI operator and judiVector May 3, 2022

ziyiyin97 requested a review from mloubout May 11, 2022 15:28

mloubout reviewed May 31, 2022

View reviewed changes

mloubout added the bug label Jun 9, 2022

ziyiyin97 force-pushed the fixabstractvec branch 2 times, most recently from 4489654 to a2360a4 Compare June 24, 2022 02:47

ziyiyin97 requested a review from mloubout June 24, 2022 03:32

mloubout reviewed Jun 24, 2022

View reviewed changes

ziyiyin97 force-pushed the fixabstractvec branch 3 times, most recently from aa59d06 to 5835aed Compare July 5, 2022 19:57

ziyiyin97 requested a review from mloubout July 5, 2022 21:39

mloubout reviewed Jul 11, 2022

View reviewed changes

ziyiyin97 added 8 commits July 12, 2022 08:39

fix mul! add associated test

a5e5846

update example

e6c4834

clean up

ae4d742

fix example

488c4ff

use only 1 source for memory requirement

26e2532

update test linear algebra to use dm.n

1a633cb

get rid of ambiguity

cecc5bd

clean up test

b420402

ziyiyin97 added 5 commits July 12, 2022 08:39

add lsqr example for lsrtm

0e0d85c

clean up

140b76f

slightly longer example running

6a94f3f

fix dispatch

442a405

run example w/ full batch

8ae4be8

mloubout force-pushed the fixabstractvec branch from b5d27da to 444bd64 Compare July 12, 2022 12:39

mloubout force-pushed the fixabstractvec branch 2 times, most recently from 867f9d4 to d4d1aab Compare July 12, 2022 13:43

examples: switch to segy_scan for larger datasets

7f2b6c3

mloubout force-pushed the fixabstractvec branch from d4d1aab to 7f2b6c3 Compare July 12, 2022 13:55

mloubout added enhancement and removed bug labels Jul 12, 2022

mloubout merged commit 405115d into master Jul 12, 2022

mloubout deleted the fixabstractvec branch July 12, 2022 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix mul! for JOLI operator and judiVector #114

fix mul! for JOLI operator and judiVector #114

ziyiyin97 commented May 3, 2022

codecov bot commented May 3, 2022 •

edited

Loading

mloubout left a comment

mloubout May 3, 2022

ziyiyin97 May 4, 2022

mloubout May 3, 2022

ziyiyin97 May 3, 2022

mloubout Jun 24, 2022

mloubout May 3, 2022

mloubout May 3, 2022

mloubout May 3, 2022

ziyiyin97 May 3, 2022

mloubout May 31, 2022

mloubout Jun 24, 2022

ziyiyin97 Jun 24, 2022

mloubout Jun 24, 2022

mloubout Jun 24, 2022

ziyiyin97 commented Jun 24, 2022

mloubout commented Jun 27, 2022

ziyiyin97 commented Jul 11, 2022

mloubout Jul 11, 2022

ziyiyin97 commented Jul 11, 2022

ziyiyin97 commented Jul 11, 2022

mloubout commented Jul 12, 2022

mloubout commented Jul 12, 2022

fix mul! for JOLI operator and judiVector #114

fix mul! for JOLI operator and judiVector #114

Conversation

ziyiyin97 commented May 3, 2022

codecov bot commented May 3, 2022 • edited Loading

Codecov Report

mloubout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyiyin97 commented Jun 24, 2022

mloubout commented Jun 27, 2022

ziyiyin97 commented Jul 11, 2022

Choose a reason for hiding this comment

ziyiyin97 commented Jul 11, 2022

ziyiyin97 commented Jul 11, 2022

mloubout commented Jul 12, 2022

mloubout commented Jul 12, 2022

codecov bot commented May 3, 2022 •

edited

Loading