slimgroup / JUDI.jl

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

Fix implementation of cumsum and diff function on judiVector to be dimensional #50

Closed ziyiyin97 closed 3 years ago

ziyiyin97 commented 3 years ago

I gave a second thought to the implementation of cumsum and diff on judiVector and I thought they should be dimensional, just like what norm and inner product do. So, depending on integration along time or along receivers, I multiply the term dt or drec to the cumsum result. Similarly, in the case of differentiation, I divide by the term dt or drec.

Also, there was a problem on diff function while dims=2. I fix that in this PR and add more associated tests to thoroughly test every case.

Appreciate any advice and review!

codecov[bot] commented 3 years ago

Codecov Report

Merging #50 (94664f7) into master (f22170d) will increase coverage by 0.03%. The diff coverage is 84.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   76.84%   76.88%   +0.03%     
==========================================
  Files          33       33              
  Lines        2626     2643      +17     
==========================================
+ Hits         2018     2032      +14     
- Misses        608      611       +3     
Impacted Files Coverage Δ
src/TimeModeling/TimeModeling.jl 100.00% <ø> (ø)
src/TimeModeling/judiVector.jl 86.60% <84.84%> (-0.17%) :arrow_down:

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 f22170d...94664f7. Read the comment docs.

ziyiyin97 commented 3 years ago

Thanks for the feedback! Based on our discussion, I only leave the time derivative/integration and report exception on other dimensions because receivers might not be uniform spaced. Also fix the tests.

ziyiyin97 commented 3 years ago

Thanks for your feedback. Fix cumsum/diff on 2nd (receiver) to be non-dimensional, e.g. sample interval = 1.

ziyiyin97 commented 3 years ago

Thanks for your suggestions! To make it clean,

  1. I overloaded lmul!, rmul!, ldiv!, rdiv! functions for judiVector so that they could be easily deployed for cumsum.
  2. I modify scalar * and / operations so that they don't have either to loop over shots or to allocate extra memory.
  3. I modified cumsum function as such.
  4. I added associated tests with them in test_judiVector.jl.
ziyiyin97 commented 3 years ago

Great suggestions! I've modified the error messages for +-*/ operations. Division by zero is classified as a DivideError() Operations with OOC object is classified as a DomainError().

ziyiyin97 commented 3 years ago

ldiv!, rdiv! are not supported in Julia 1.1 version. Thus, switch to a try/catch framework.

ziyiyin97 commented 3 years ago

Modified ldiv, rdiv functions for judiVector. Also remove test_wavefield.jl file from src/TimeModeling/ folder since I don't think it is used anywhere.

ziyiyin97 commented 3 years ago

Use selectdim in diff which makes it cleaner with dimensions and avoids extra space allocation

ziyiyin97 commented 3 years ago

Modify tests for lmul, rmul, ldiv, rdiv to be more explicit.