Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

examples: Fix opt arg-passing for elastic and TTI #1425

Merged
merged 1 commit into from
Aug 27, 2020
Merged

Conversation

georgebisbas
Copy link
Contributor

Fixes the opt argument-passing to TTI and elastic.

@georgebisbas georgebisbas self-assigned this Aug 7, 2020
@georgebisbas georgebisbas changed the title examples: Add kwargs to operators and wavesolvers examples: Fix opt arg-passing for elastic and TTI Aug 7, 2020
@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #1425 into master will decrease coverage by 0.96%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1425      +/-   ##
==========================================
- Coverage   87.01%   86.05%   -0.97%     
==========================================
  Files         188      188              
  Lines       27072    27072              
  Branches     3674     3674              
==========================================
- Hits        23557    23297     -260     
- Misses       3089     3349     +260     
  Partials      426      426              
Impacted Files Coverage Δ
examples/seismic/acoustic/wavesolver.py 100.00% <ø> (ø)
examples/seismic/elastic/wavesolver.py 100.00% <ø> (ø)
examples/seismic/self_adjoint/wavesolver.py 100.00% <ø> (ø)
examples/seismic/tti/wavesolver.py 79.36% <ø> (-17.47%) ⬇️
examples/seismic/viscoacoustic/wavesolver.py 0.00% <ø> (-100.00%) ⬇️
examples/seismic/viscoelastic/wavesolver.py 100.00% <ø> (ø)
examples/seismic/elastic/operators.py 100.00% <100.00%> (ø)
examples/seismic/tti/tti_example.py 35.13% <100.00%> (-21.63%) ⬇️
examples/seismic/viscoacoustic/operators.py 0.00% <0.00%> (-100.00%) ⬇️
... and 10 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 378e2c6...d03cb9c. Read the comment docs.

@@ -52,7 +52,7 @@ def op_fwd(self, kernel='centered', save=False):
"""Cached operator for forward runs with buffered wavefield"""
return ForwardOperator(self.model, save=save, geometry=self.geometry,
space_order=self.space_order,
kernel=kernel, **self._kwargs)
**self._kwargs)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE FR: This kernel arg causes memoization error if I keep it.
NOt sure how to solve it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYM memoization error, please put it back so can be debugged it should be there (and in adjoint as well)

@@ -17,15 +18,16 @@ def tti_setup(shape=(50, 50, 50), spacing=(20.0, 20.0, 20.0), tn=250.0,
# Source and receiver geometries
geometry = setup_geometry(model, tn)

return AnisotropicWaveSolver(model, geometry, space_order=space_order)
return AnisotropicWaveSolver(model, geometry, space_order=space_order,
kernel=kernel, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not used in the init of the solver so no kernel shouldn't be there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought your previous comment meant that I should add it here (?).
Which one is the signature?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant need to be in the signature of tti_setup so that doesn't end up in kwargs

@@ -52,7 +52,7 @@ def op_fwd(self, kernel='centered', save=False):
"""Cached operator for forward runs with buffered wavefield"""
return ForwardOperator(self.model, save=save, geometry=self.geometry,
space_order=self.space_order,
kernel=kernel, **self._kwargs)
**self._kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYM memoization error, please put it back so can be debugged it should be there (and in adjoint as well)

@georgebisbas georgebisbas force-pushed the optkwargs branch 2 times, most recently from 467f713 to 166aa9d Compare August 26, 2020 13:38
Copy link
Contributor

@FabioLuporini FabioLuporini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mloubout what's missing here?

@rhodrin rhodrin merged commit 5d92539 into master Aug 27, 2020
@FabioLuporini FabioLuporini deleted the optkwargs branch August 31, 2020 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants