search

slimgroup / JUDI.jl

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

Added possibility to set `dt,t` to OOC Geometry #139

Closed kerim371 closed 1 year ago

kerim371 commented 2 years ago

When doing forward modeling sometimes we want to set geometry from SEGY and arbitrary dt,nt. This allows to explore the same model with different time length without modifying SEGY. To provide such opportunity opt args dt,t added to OOC Geometry constructors. getIndex was modified to pass these parameters as args. When loading OOC geom to IC we now copy dt,nt,t from OOC rather than getting it from SEGY.

With this we now need to make checks in judiVector in case the user sets geom that doesn't corresponds to data (data is needed for migration/inversion)

kerim371 commented 2 years ago

There are four constructors for judiVector that probably should do the checks for at least Geometry's and data's t,nt,dt. But I can't figure out the style that I need to do that. For example if I iterate through all the sources from OOC geometry and data to check dt,nt,t that may be pretty slow (I guess) because the OOC data may be too big.

codecov[bot] commented 2 years ago

Codecov Report

Base: 83.96% // Head: 76.11% // Decreases project coverage by -7.85% :warning:

Coverage data is based on head (ae792bd) compared to base (b17e604). Patch coverage: 94.02% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #139 +/- ## ========================================== - Coverage 83.96% 76.11% -7.86% ========================================== Files 24 24 Lines 1940 1972 +32 ========================================== - Hits 1629 1501 -128 - Misses 311 471 +160 ``` | [Impacted Files](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup) | Coverage Δ | | |---|---|---| | [src/TimeModeling/Types/GeometryStructure.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL1RpbWVNb2RlbGluZy9UeXBlcy9HZW9tZXRyeVN0cnVjdHVyZS5qbA==) | `91.98% <93.65%> (-0.84%)` | :arrow_down: | | [src/TimeModeling/Types/judiVector.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL1RpbWVNb2RlbGluZy9UeXBlcy9qdWRpVmVjdG9yLmps) | `92.18% <100.00%> (+0.25%)` | :arrow_up: | | [src/rrules.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL3JydWxlcy5qbA==) | `0.00% <0.00%> (-83.34%)` | :arrow_down: | | [src/TimeModeling/Modeling/distributed.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL1RpbWVNb2RlbGluZy9Nb2RlbGluZy9kaXN0cmlidXRlZC5qbA==) | `32.35% <0.00%> (-61.77%)` | :arrow_down: | | [src/TimeModeling/LinearOperators/lazy.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL1RpbWVNb2RlbGluZy9MaW5lYXJPcGVyYXRvcnMvbGF6eS5qbA==) | `46.05% <0.00%> (-39.85%)` | :arrow_down: | | [src/TimeModeling/Modeling/misfit\_fg.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL1RpbWVNb2RlbGluZy9Nb2RlbGluZy9taXNmaXRfZmcuamw=) | `62.50% <0.00%> (-30.36%)` | :arrow_down: | | [src/TimeModeling/Modeling/python\_interface.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL1RpbWVNb2RlbGluZy9Nb2RlbGluZy9weXRob25faW50ZXJmYWNlLmps) | `82.64% <0.00%> (-16.53%)` | :arrow_down: | | [src/TimeModeling/LinearOperators/operators.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL1RpbWVNb2RlbGluZy9MaW5lYXJPcGVyYXRvcnMvb3BlcmF0b3JzLmps) | `69.02% <0.00%> (-12.56%)` | :arrow_down: | | [src/TimeModeling/TimeModeling.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL1RpbWVNb2RlbGluZy9UaW1lTW9kZWxpbmcuamw=) | `88.23% <0.00%> (-11.77%)` | :arrow_down: | | [src/TimeModeling/Utils/auxiliaryFunctions.jl](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup#diff-c3JjL1RpbWVNb2RlbGluZy9VdGlscy9hdXhpbGlhcnlGdW5jdGlvbnMuamw=) | `71.07% <0.00%> (-7.39%)` | :arrow_down: | | ... and [3 more](https://codecov.io/gh/slimgroup/JUDI.jl/pull/139/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slimgroup)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mloubout commented 2 years ago

Thanks. Will have a look asap. SEG conference in a week so bit too busy but not forgetting about it.

kerim371 commented 2 years ago

Thanks. Will have a look asap. SEG conference in a week so bit too busy but not forgetting about it.

Good luck there! The PR may wait, it is not much important.

kerim371 commented 1 year ago

I tried to follow ypur recommecndations.

Please check check_geom function. Does it need to be exported? And if Julia allows to call function that is not implemented yet (if it is implemented below the caller function) I would move check_geom somewhere after judiVector constructors.

Also could you tell me where I can put tests?

mloubout commented 1 year ago

Does it need to be exported?

No it's an internal check so shouldn't be exported

I would move check_geom to Geometry.jl and everything should be properly ordered in term of definition.

For the tests I'd add one test in test_geometry for the geometry construction and check_geom and one test in test_judiVector for the judiVector creation (and maybe add a check that makes sure it errors if not compatible)

If you don't mind, once you added the test I may push a commit myself to make the syntax a bit more julia-esque consistent with the rest of JUDI. But I can also do that after merging your PR if you prefer this to be only your commits.

kerim371 commented 1 year ago

If you don't mind, once you added the test I may push a commit myself to make the syntax a bit more julia-esque consistent with the rest of JUDI. But I can also do that after merging your PR if you prefer this to be only your commits.

I don't mind, that is completely fine.

I will add tests soon

kerim371 commented 1 year ago

I added test to test_judiVector.jl I think testing check_geom function will lead to code duplication: check_geom only throws exception if geometry's ns != data's ns

mloubout commented 1 year ago

Just gonna go ahead and merge I can clean it up later.