Skip to content

Conversation

@vincepandolfo
Copy link
Contributor

Changed the time_substitutions method to build a dictionary of changes to be made by Sympy.subs before calling it instead of calling subs every time as suggested by @mlange05

Copy link

Choose a reason for hiding this comment

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

Could we please use SymPy's built-in generator traversals sympy.preorder_traversal instead of actually recursing? That should make things even faster.

@mlange05
Copy link

mlange05 commented Aug 1, 2016

Yes, this looks convincing. Can we get an indication of how much faster this is? I would like @navjotk to have a second look at this, but otherwise I'm happy.

@vincepandolfo
Copy link
Contributor Author

On my machine @mloubout example (https://github.com/opesci/devito/blob/Zhang_tti/examples/tti_example2D.py) is ~6 times faster with this change

@FabioLuporini
Copy link
Contributor

looks reasonable to me

@navjotk
Copy link
Member

navjotk commented Aug 2, 2016

The change looks good on visual inspection. However, this bit of the code is not currently being tested (I believe). I think now is as good a time as any to add a test for this.

Here's my suggestion for the test: Run a forward propagation with save=True and another with save=False. A comparison of the last time step in both cases should be the assert for the test. Such a test would be able to pick up bugs in this bit of code.

@navjotk navjotk mentioned this pull request Aug 2, 2016
@vincepandolfo vincepandolfo force-pushed the time_substitution_optimization branch 2 times, most recently from cca351f to 1d25b64 Compare August 2, 2016 14:23
@FabioLuporini
Copy link
Contributor

FabioLuporini commented Aug 3, 2016

The test you asked is landing with #67 . Once that PR lands, are you happy with merging this @navjotk ?

@vincepandolfo vincepandolfo force-pushed the time_substitution_optimization branch from 1d25b64 to 2d99698 Compare August 3, 2016 15:59
@vincepandolfo vincepandolfo force-pushed the time_substitution_optimization branch from 2d99698 to ca2612a Compare August 4, 2016 08:50
@navjotk
Copy link
Member

navjotk commented Aug 4, 2016

Yes. Now that we have the test in place, we are good to go on this. Merging.

@navjotk navjotk merged commit b3f19c0 into master Aug 4, 2016
@navjotk navjotk deleted the time_substitution_optimization branch August 4, 2016 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants