remove xsum_kernels (to be replaced with xcombine_sem, which we should rename xcombine_kernels)

[rmodrak]

Switching to command argument passing as a way of specifying kernel names in sum_kernels and sum_preconditioned_kernels would be helpful for all those working with alternate material parameterizations (acoustic, anisotropic, Thomsen, ChenTromp). Currently, kernel names are specified via hardwired parameter lists.

If no one objects, I will go ahead and submit one or two pull request that include the following two changes:

• make smoothing and summing utilities share a consistent command line interface
• allow smoothing and summing utilties to act on multiple elastic parameters by accepting comma delimited list of kernel names

As another benefit, the cost of smoothing operations could be reduced by making smooth_sem work on multiple material parameters (as per Daniel's comments). The second change above constitutes a step in this direction.

This issue applies to both SPECFEM3D and SPECFEM3D_GLOBE.

[rmodrak]

UPDATE: In the end, rather than modifying xsum_kernels, I’ve created a new utility xcombine_sem that adds together multiple kernels. (See pending pull request.)

xcombine_sem reads paths and material parameter names from the command line, with an interface very similar to xsmooth_sem.

xcombine_sem provides all the functionality of xsum_kernels. To avoid code duplication maintenance issues, we might consider deprecating the latter.

[komatits]

Yes, let us remove xsum_kernels then. Could you do it when you have time? (and update the users manual or the corresponding README file accordingly); and then close this Git issue. Thanks, Dimitri.

[komatits]

It would probably be better to call it xcombine_kernels rather than xcombine_sem though (because what it combines is kernels). "SEM" is a method, not a result.

[casarotti]

since I'm using it in a production mode... can we leave the old version for a while? we can create a "DEPRECATED" section....

On Sun Feb 22 2015 at 12:12:19 Dimitri Komatitsch notifications@github.com wrote:

It would probably be better to call it xcombine_kernels rather than xcombine_sem though (because what it combines is kernels). "SEM" is a method, not a result.

— Reply to this email directly or view it on GitHub https://github.com/geodynamics/specfem3d/issues/419#issuecomment-75430460 .

[rmodrak]

UPDATE: Because of a request to retain to xsum_kernels in its original form somewhat longer, I’ve created a new utility xcombine_sem that adds together models or kernels and restored the version of xsum_kernels that existed prior to any of my changes. To avoid code duplication issues, I agree, we should deprecate and ultimately remove xsum_kernels.

Also, I agree, there are probably better conventions than the suffix “sem”. However, use of the suffix is already established in the code, especially the global version. It’s a convention that Daniel created to indicate that particular routines share a similar command line interface. Also, although the primary use case for these utilities is to act on kernels, the utilities are general enough that they can in fact be applied to any scalar field defined on GLL points.

For consistency, I’d suggest that for now we continue to follow Daniel’s convention. Later on, if we can decide on a better convention, we could apply it at once to all the different src/tomography routines that currently end in “sem”.

Is any of this unclear? Please let me know if there are any questions.

How does this sound to you Dimitri?

[komatits]

Sure, let us keep it and let us rename it "xsum_kernels_old_deprecated" (let us also rename the *.f90 file accordingly)

Thanks, Dimitri.

On 02/22/2015 04:01 PM, emanuele casarotti wrote:

since I'm using it in a production mode... can we leave the old version for a while? we can create a "DEPRECATED" section....

On Sun Feb 22 2015 at 12:12:19 Dimitri Komatitsch notifications@github.com wrote:

It would probably be better to call it xcombine_kernels rather than xcombine_sem though (because what it combines is kernels). "SEM" is a method, not a result.

— Reply to this email directly or view it on GitHub

https://github.com/geodynamics/specfem3d/issues/419#issuecomment-75430460 .

— Reply to this email directly or view it on GitHub https://github.com/geodynamics/specfem3d/issues/419#issuecomment-75439218.

Dimitri Komatitsch CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics, UPR 7051, Marseille, France http://komatitsch.free.fr

[komatits]

Done (renamed).

What about sum_preconditioned_kernels.f90, it is also obsolete and should we rename it as well?

PS for Ryan: I do not see your new routines in Makefile.in, are you sure they get compiled when you type "make all"?

Thanks, Dimitri.

On 02/23/2015 12:40 AM, Dimitri Komatitsch wrote:

Sure, let us keep it and let us rename it "xsum_kernels_old_deprecated" (let us also rename the *.f90 file accordingly)

Thanks, Dimitri.

On 02/22/2015 04:01 PM, emanuele casarotti wrote:

since I'm using it in a production mode... can we leave the old version for a while? we can create a "DEPRECATED" section....

On Sun Feb 22 2015 at 12:12:19 Dimitri Komatitsch notifications@github.com wrote:

It would probably be better to call it xcombine_kernels rather than xcombine_sem though (because what it combines is kernels). "SEM" is a method, not a result.

— Reply to this email directly or view it on GitHub

https://github.com/geodynamics/specfem3d/issues/419#issuecomment-75430460

.

— Reply to this email directly or view it on GitHub https://github.com/geodynamics/specfem3d/issues/419#issuecomment-75439218.

Dimitri Komatitsch CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics, UPR 7051, Marseille, France http://komatitsch.free.fr

[rmodrak]

Hi Dimitri,

Thanks for pointing that out. I was a little confused about the make system. I added the new routines to src/tomography/rules.mk, but not Makefile.in. Let me correct that.

Ryan

[QuLogic]

Instead of adding all the executables, we should just change the all target to depend on the "group" targets, e.g., tomography.

[rmodrak]

I've added the new xclip_sem and xcombine_sem utilities to Makefile.in via pull request #433.

QuLogic's idea sounds interesting. I wonder, has this idea been considered at all in the past?

[rmodrak]

Hi Dimitri,

Regarding deprecation of xsum_kernels, I wanted to ask your opinion.

xcombine_sem is a somewhat different utility than xsum_kernels. Although xcombine_sem does provide all the functionality of xsum_kernels, users would have to read the usage comments to figure out how to supply from the command line what was previously hardwired.

The main reason for deprecation and ultimately removal of xsum_kernels is to avoid code duplication; however, there are a number of more flagrant instances of code duplication in src/tomography that I have not addressed. Also, there is the issue that there is no new utility that takes the place of xsum_preconditioned_kernels.

For these reasons, I wonder if it might be best to temporarily remove the '_old_deprecated' suffix from xsum_kernels, and add it back only when an alternative exists for xsum_kernels_preconditioned.

I'd be happy to do whatever you think is most clear. If you think it is best to temporarily remove the '_old_deprecated' suffix, I could do the work of submitting the pull request myself.

Thanks, Ryan

[komatits]

Hi Ryan,

OK, no problem, let us revert that then. Ultimately as you say the goal should be to have a single set of utilities though, and based on how we handled that in SPECFEM in the past the only option is often to gradually remove the old versions otherwise people keep using them (and we get the current situation: three or four different tools for more or less the same thing...).

But as you say it will probably take a few months to reach that.

Thanks, Dimitri.

On 02/24/2015 07:39 AM, rmodrak wrote:

Hi Dimitri,

Regarding deprecation of xsum_kernels, I wanted to ask your opinion.

xcombine_sem is a somewhat different utility than xsum_kernels. Although xcombine_sem does provide all the functionality of xsum_kernels, users would have to read the usage comments to figure out how to supply from the command line what was previously was hardwired.

The main reason for deprecation and ultimately removal of xsum_kernels is to avoid code duplication; however, there are a number of more flagrant instances of code duplication in src/tomography that I have not addressed. Also, there is also the issue that there is no new utility that takes the place of xsum_preconditioned_kernels.

For these reasons, I wonder if it might be best to temporarily remove the '_old_deprecated' suffix from xsum_kernels, and add it back only when an alternative exists for xsum_kernels_preconditioned as well.

I'd be happy to do whatever you think is most clear. If you think it is best to temporarily remove the '_old_deprecated' suffix, I could do the work of submitting the pull request myself.

Ryan

— Reply to this email directly or view it on GitHub https://github.com/geodynamics/specfem3d/issues/419#issuecomment-75707216.

Dimitri Komatitsch CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics, UPR 7051, Marseille, France http://komatitsch.free.fr

[rmodrak]

I think this issue has been addressed. If there are no further comments, I'll go ahead and close it shortly.