Use a separate class for M2L translation and change expansion default to FFT#113
Conversation
| from sumpy.expansion.m2l import VolumeTaylorM2LWithFFT | ||
| m2l_translation = VolumeTaylorM2LWithFFT() | ||
| else: | ||
| m2l_translation = None |
There was a problem hiding this comment.
Maybe default this to on in the cases where it works? (of course keeping a way to force which is used)
|
Could you take a look? This now seems to fail CI. |
|
This is because I changed the default for the M2L translation to FFT and now all tests require the additional translation_classes data. How about I keep the default to non FFT in sumpy and use the FFT in pytential where the translation_classes is computed and passed? |
|
I'd prefer to have the default at FFT while modifying all the constructor calls in sumpy. I could also live with a non-FFT default that yells about its deprecation. (To be clear: We won't deprecate non-FFT execution, just not specifying which you want.) |
isuruf
changed the title
|
Factor of 2 change fixed the tests. Once inducer/pytential#102 is merged this should be ready for review. |
Out of curiosity: where was that factor of 2? Also, for my peace of mind: There are no functional changes here, correct? |
There's the factor of 2 change in rscale and the change of default for M2L translation type to FFT. |
inducer
left a comment
inducer
left a comment
There was a problem hiding this comment.
This was hard to review because of the amount of code that's moved. But pending your confirmation that there are no functional changes and that the factor of 2 is innocuous, this LGTM. Thanks for working on this!
|
Didn't see your response until after I submitted the review. Thanks for the summary! LGTM, in it goes. |
|
(And thanks for working on this!) |
2 participants
Fixes #104