slimgroup / JUDI.jl

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

fix reduce #77

Closed ziyiyin97 closed 2 years ago

ziyiyin97 commented 2 years ago

Fixes #76

@ziyiyin97 I told you multiple times how to tag an issue in a PR!

codecov[bot] commented 2 years ago

Codecov Report

Merging #77 (3b89a83) into master (b6384c5) will decrease coverage by 9.19%. The diff coverage is 100.00%.

:exclamation: Current head 3b89a83 differs from pull request most recent head 0933215. Consider uploading reports for the commit 0933215 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   84.46%   75.26%   -9.20%     
==========================================
  Files          30       30              
  Lines        2336     2333       -3     
==========================================
- Hits         1973     1756     -217     
- Misses        363      577     +214     
Impacted Files Coverage Δ
src/TimeModeling/Modeling/distributed.jl 76.31% <100.00%> (-21.06%) :arrow_down:
...rc/TimeModeling/Modeling/lsrtm_objective_serial.jl 0.00% <0.00%> (-79.32%) :arrow_down:
...TimeModeling/LinearOperators/judiExtendedSource.jl 17.64% <0.00%> (-58.83%) :arrow_down:
...eling/Modeling/extended_source_interface_serial.jl 47.05% <0.00%> (-29.42%) :arrow_down:
...ling/LinearOperators/judiJacobianExtendedSource.jl 64.00% <0.00%> (-28.00%) :arrow_down:
...rc/TimeModeling/LinearOperators/judiPDEextended.jl 48.78% <0.00%> (-26.83%) :arrow_down:
src/TimeModeling/Modeling/python_interface.jl 65.71% <0.00%> (-26.67%) :arrow_down:
src/TimeModeling/LinearOperators/judiLRWF.jl 64.70% <0.00%> (-17.65%) :arrow_down:
src/TimeModeling/Modeling/time_modeling_serial.jl 70.00% <0.00%> (-15.00%) :arrow_down:
... and 14 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 b6384c5...0933215. Read the comment docs.

mloubout commented 2 years ago

Small (don't need Judi) test would be nice but looks fine. Change version to 2.6.1

ziyiyin97 commented 2 years ago

Emm still ain't correct. I've taken another look with a 5 source example -- it seems to mess up the order of 2, 4, 5-th shot

ziyiyin97 commented 2 years ago

What kind of test do you @mloubout want to add BTW? If we want to make sure it's 100% working well, we have to add one more dummy function to _parallel_functions. Do you have any better suggestion?

mloubout commented 2 years ago

No you don't just loop spawn then reduce where you know the result

ziyiyin97 commented 2 years ago

Oh I seems to understand this behavior: the reduce step for time modeling is basically a stack of judiVectors. During each step of the reduce, we have to stack the neighbors because the order really matters. So this line is https://github.com/slimgroup/JUDI.jl/blob/dc8d9f2471cdbd3693573b85d3cd3409de96e168/src/TimeModeling/Modeling/distributed.jl#L240 problematic.

Basically if we have 4 sources, we have to first stack 1,2 and 3,4 in the first round, then stack 1,3 in the 2nd round. Now it's stacking 1,3 and 2,4 in the first round, so its ordering messed up.

mloubout commented 2 years ago

Yes basically should only need to change the loops indices to fix it. I.e reduce last m then loop every two and reduce next

ziyiyin97 commented 2 years ago

Then do you have any suggestion on how to reduce the remainder, i.e. reduce 5,6,7 when there are 7 futures

ziyiyin97 commented 2 years ago

My current strategy is (for 7 futures):

first stack 1,2 ; 3,4 ; 5,6 (since there are 3 remainders, we stack 3 times)

then the ones to be stacked next are 1,3,5 (from previous round) and 7 (who didn't get stacked in the previous round)

mloubout commented 2 years ago

You just have to change the loop index why are you making things complicated

mloubout commented 2 years ago
for i=1:2:2*nleaf
remotecall_fetch(local_reduce!, futures[i].where, futures[i], futures[i+1])

And that's all it takes.

ziyiyin97 commented 2 years ago

No in the 2nd level, i should talk to i+2 not i+1 any more

mloubout commented 2 years ago

deleteat! after loop

ziyiyin97 commented 2 years ago

OK deleteat should be the cleanest way! Thanks

ziyiyin97 commented 2 years ago

Rebased