slimgroup / JUDI.jl

Julia Devito inversion.
https://slimgroup.github.io/JUDI.jl
MIT License
96 stars 30 forks source link

Fix on geometries and seisblock #69

Closed ziyiyin97 closed 2 years ago

ziyiyin97 commented 2 years ago

Fix #85

JUDI started to use Array{Array{Float32, 1}, 1} for geometry on yloc (a couple of versions ago) so the fix on rounding is necessary

codecov[bot] commented 2 years ago

Codecov Report

Merging #69 (d6619bc) into master (1ed8989) will increase coverage by 0.05%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
+ Coverage   84.58%   84.64%   +0.05%     
==========================================
  Files          30       30              
  Lines        2336     2344       +8     
==========================================
+ Hits         1976     1984       +8     
  Misses        360      360              
Impacted Files Coverage Δ
src/TimeModeling/Types/GeometryStructure.jl 93.78% <100.00%> (+0.15%) :arrow_up:
src/TimeModeling/Types/judiVector.jl 94.92% <100.00%> (+0.05%) :arrow_up:

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 1ed8989...d6619bc. Read the comment docs.

ziyiyin97 commented 2 years ago

It is not tested because https://github.com/slimgroup/JUDI.jl/blob/16171876a379cc372b1d165483b1f1a3d790c984/src/TimeModeling/Types/judiVector.jl#L439 this line is wrong for the unit test shot records (which is 3D data)

ziyiyin97 commented 2 years ago

I don't think there is any 2D shot record (i.e. yloc has different lengths than xloc or zloc) under data folder so the weird case is not tested. Let me know if we still want to add a test for it (then in this case, what would be a clean way to acquire this 2D shot record). I've added test to src_to_SeisBlock

mloubout commented 2 years ago

You don't need a existing file, just create a block and check it's valid.

& commits for these changes is not good. Please rebase and keep the commit history clean

ziyiyin97 commented 2 years ago
  1. since xloc, yloc, zloc have changed to be Array{Array{Float32,1},1} for sources, some test scripts should be changed
  2. dispatch the data in judiVector so if 1D array is input, it reshapes it to 2D array. This solves careless mistakes like #44
  3. Made some edits on when judiVector is converted to Seisblock (or the other way around) based on the new yloc in source/ receiver
ziyiyin97 commented 2 years ago

See #85

ziyiyin97 commented 2 years ago

OK will fix that. But after fixing that, when you follow #85, you will find

julia> dobs.geometry.yloc
2-element Vector{Vector{Float32}}:
 [0.0]
 [0.0]
julia> judiVector(block).geometry.yloc
2-element Vector{Vector{Float32}}:
 [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0  …  0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]
 [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0  …  0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]

That's why we need to do this https://github.com/slimgroup/JUDI.jl/blob/0233edb25f605f56a90066ef35fce4b6622d701a/src/TimeModeling/Types/GeometryStructure.jl#L196

and then if we added this, we run the tests, we will get another error at https://github.com/slimgroup/JUDI.jl/blob/4675593bc4183a5a2ce34de73df75b4f946e6edb/test/test_judiVector.jl#L341

because

julia> d_block.geometry.yloc
2-element Vector{Vector{Float32}}:
 [0.0]
 [0.0]

and

julia> d_get.geometry.yloc
2-element Vector{Vector{Float32}}:
 [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0  …  0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]
 [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0  …  0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]

That's why I fixed Geometry(::GeometryOOC). Then the next problem is at src_geometry = Geometry(block; key="source", segy_depth_key="SourceSurfaceElevation") https://github.com/slimgroup/JUDI.jl/blob/4675593bc4183a5a2ce34de73df75b4f946e6edb/test/test_judiVector.jl#L200 basically we get

julia> src_geometry.xloc
1-element Vector{Float32}:
 100.0

julia> src_geometry.yloc
1-element Vector{Float32}:
 0.0

julia> src_geometry.zloc
1-element Vector{Float32}:
 20.0

this is wrong because we expect to have src_geometry.xloc as Vector{Vector{Float32}}. That's why I edited the Geometry(::SeisBlock) and also edited the associated tests.

@mloubout basically I made these edits to make sure the geometry.xloc is Vector{Vector{Float32}}, andgeometry.ylochas only[[0],[0]]` in 2D

mloubout commented 2 years ago

Ok so everything about the coordinates makes sense thanks for the details.

I'll have another fresh look but overall I think that [1:1] and wavelet shape thingy is the only thing I think

ziyiyin97 commented 2 years ago

Sure. I'll rebase after all things are sorted out

ziyiyin97 commented 2 years ago

srcGeometry.xloc should be Vector{Vector{T}}, e.g. [[100f0],[200f0]] but now it's not -- https://github.com/slimgroup/JUDI.jl/pull/69#discussion_r791809809 you can see here that now it is Vector{T}. We should enforce the source and receive geometries to be set in a standard way so that converting it to SEGY file and back things are still reasonable. We don't want 2 judiVectors with different geometries while they actually correspond to the same source, right?

mloubout commented 2 years ago

super-seeded by #91