slimgroup / JUDI.jl

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

Pmap parallelization over sources and vintages #67

Closed ziyiyin97 closed 3 years ago

ziyiyin97 commented 3 years ago
  1. overload lsrtm_objective, fwi_objective to parallel over sources/vintages using pmap
  2. fix previous lsrtm_objective test
  3. add a subsample function for Info
  4. add associated tests
  5. update judipmap to take more indices
codecov[bot] commented 3 years ago

Codecov Report

Merging #67 (1c46aef) into master (be60331) will increase coverage by 0.14%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   83.49%   83.63%   +0.14%     
==========================================
  Files          35       35              
  Lines        2320     2341      +21     
==========================================
+ Hits         1937     1958      +21     
  Misses        383      383              
Impacted Files Coverage Δ
...rc/TimeModeling/Modeling/fwi_objective_parallel.jl 100.00% <100.00%> (ø)
src/TimeModeling/Modeling/fwi_objective_serial.jl 92.00% <100.00%> (+0.69%) :arrow_up:
.../TimeModeling/Modeling/lsrtm_objective_parallel.jl 100.00% <100.00%> (ø)
...rc/TimeModeling/Modeling/lsrtm_objective_serial.jl 88.00% <100.00%> (+1.04%) :arrow_up:
src/TimeModeling/Modeling/utils.jl 100.00% <100.00%> (ø)
src/TimeModeling/Types/InfoStructure.jl 91.66% <100.00%> (+1.66%) :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 be60331...1c46aef. Read the comment docs.

mloubout commented 3 years ago

Told you there is much cleaner and nicer way to to do it. Argument name should reflect type better so rename model::Array

ziyiyin97 commented 3 years ago

If we overload lsrtm_objective this way, it seems we will definitely need to overload lsrtm_objective in JUDI4Cloud. This is because:

a function call lsrtm_objective with multiple vintages in JUDI4Cloud calls judipmap((m, q, d, dm) -> lsrtm_objective(m, q, d, dm, options; nlind=nlind), model, source, dObs, dm), then following judipmap, it will do sync & async & pmap on local machine (there is no room for @batchexec to come in)

mloubout commented 3 years ago

You are still thinking about your one little application rather than the big pictures. You once again added duplicate copy-paste code rather than think about the different levels. THe Array version of judipmap should not have pmap in it. And then, if done properly, you only need to redefine/overload a single judipmap in jUDI4Cloud and everything works perfectly.

You need to try to forget about your one application and think about what is the general software problem and what design fits it better.

ziyiyin97 commented 3 years ago

Overloaded judipmap so that it takes any kind of function, iterables and it will always work

mloubout commented 3 years ago

no

mloubout commented 3 years ago

Clean up commit history and fix indentation in test and should be GTG