SPECFEM / specfem3d_globe

SPECFEM3D_GLOBE simulates global and regional (continental-scale) seismic wave propagation.
GNU General Public License v3.0
90 stars 95 forks source link

simplify region codes in src/postprocess #338

Closed rmodrak closed 9 years ago

rmodrak commented 9 years ago

Dear all,

Currently, in src/postprocess, there are three different conventions concerning region prefixes. Sometimes region prefixes are hardwired into code (e.g. reg_name = '_reg1_'). Other times a constants file is used (e.g. use postprocess_par,only: reg). Other times a prefix is determined based on a command line argument.

To make things simpler, I'd like move to a single convention: require users to include the region code explicitly through the command line arguments. ​ old convention

mpirun bin/xsmooth_sem 10. 10. rho_kernel INPUT_DIR OUTPUT_DIR

proposed new convention

mpirun bin/xsmooth_sem 10. 10. reg1_rho_kernel INPUT_DIR OUTPUT_DIR

This change would not affect any SPECFEM3D utilities, and only one of the 'classic' SPECFEM3D_GLOBE utilities, smooth_sem.f90, would be affected. (If Daniel wants me to modify the recently added utilities addition_sem and difference_sem utilities, I could do that as well.)

If there are any concerns, please let me know.

This is the final proposed change concerning the postprocess utilities--thanks for bearing with me.

Ryan

komatits commented 9 years ago

Hi Ryan,

What about suppressing the region suffix from the command line and smoothing all the regions (i.e. reg1 reg2 reg3) automatically (if the corresponding kernel exists on the disk, if not, ignoring that region).

Otherwise this would make the syntax differ from SPECFEM3D, and I am not sure someone would smooth a region without smoothing the other (?). Of course some kernels are in the crust_mantle region only, if so for these kernels reg2 and reg3 are meaningless; (but then they can probably be smoothed anyway even if that is useless, because smoothing reg2 is cheap and smoothing reg3 is very cheap).

Anyway, if possible let us avoid making the syntax of GLOBE tools differ from the rest, since we are trying to do the opposite.

Other option: by default smooth all the regions if no "reg" argument is present on the command line, but accept an optional "reg" argument if the user wants to smooth a specific region only; as long as it is optional we do not break compatibility with SPECFEM3D syntax.

Thanks, Dimitri.

On 03/03/2015 10:09 PM, rmodrak wrote:

Dear all,

Currently, in src/postprocess, there are three different conventions concerning region suffixes. Sometimes region prefixes are hardwired into code (e.g. |reg_name = 'reg1'|). Other times a constants file is used (e.g. |use postprocess_par,only: reg|). Other times a prefix is determined based on a command line argument.

To make things simpler, I'd like move to a single convention: require users to include the region code explicitly through the command line arguments. ​ old convention

bin/xsmooth 10. 10. rho_kernel INPUT_DIR OUTPUT_DIR

proposed new convention

bin/xsmooth 10. 10. reg1_rho_kernel INPUT_DIR OUTPUT_DIR

This change would not affect any SPECFEM3D utilities, and only one of the 'classic' SPECFEM3D_GLOBE utilities, |smooth_sem.f90|, would be affected. (If Daniel wants to modify the recently added utilities |addition_sem| and |difference_sem| utilities, I could do that as well.)

If there are any concerns, please let me know.

This is the final proposed change concerning the postprocess utilities--thanks for bearing with me.

Ryan

— Reply to this email directly or view it on GitHub https://github.com/geodynamics/specfem3d_globe/issues/338.

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

rmodrak commented 9 years ago

Hi Dimitri,

That is an excellent point. Thanks very much for the feedback.

There is a sense in which the proposed change improves consistency between packages by removing a hardwired variable from SPECFEM3D_GLOBE that is not present in SPECFEM3D.

On the other hand though, I do agree very much with your view.

If I could, let me think more about this from the standpoint of trying to interface with xsmooth_sem from an external script. Also, let me follow up with Daniel about this, since I brought it up with him earlier.

I'll report back later this week, and in the meantime I'll hold off on any changes.

Thanks, Ryan

komatits commented 9 years ago

Hi Ryan,

Perfect!

Using an optional argument could be an option; I am not so sure about using an external script (doing that would force users to change the way they use these codes, i.e. currently calling them directly from the command line; if possible, let us not change that).

Thanks, Dimitri.

On 03/04/2015 06:14 AM, rmodrak wrote:

Hi Dimitri,

That is an excellent point. Thanks very much for the feedback.

There is a sense in which the proposed change improves consistency between packages by removing a hardwired variable from SPECFEM3D_GLOBE that is not present in SPECFEM3D.

On the other hand though, I do agree very much with your view.

If I could, let me think more about this from the standpoint of trying to interface with xsmooth_sem from an external script. Also, let me follow up with Daniel about this, since I brought it up with him earlier.

I'll report back later this week, and in the meantime I'll hold off on any changes.

Thanks, Ryan

— Reply to this email directly or view it on GitHub https://github.com/geodynamics/specfem3d_globe/issues/338#issuecomment-77098510.

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

rmodrak commented 9 years ago

Hi Dimitri,

Sorry, all I meant was, anyone using these utilities must call them from an external script, typically a bash script. This is the approach established by Carl and Daniel.

Of course, we need to try to avoid breaking existing command line interfaces. But if we can extend command line interfaces in a way that provides more flexibility or that anticipates future user needs, there are real benefits for everyone.

Ryan

rmodrak commented 9 years ago

Hi Dimitri,

If it's alright, why don't we just leave everything as it is and go ahead and close this issue.

Thanks, Ryan

komatits commented 9 years ago

Hi Ryan,

Ah, OK, then we are all set. So let us implement your initial idea (getting rid of hardwired region codes and adding that to the kernel name in the command line arguments). That will make the current routines much cleaner.

Thanks, Dimitri.

On 03/04/2015 02:39 PM, rmodrak wrote:

Hi Dimitri,

Sorry, all I meant was, anyone using these utilities must call them from an external script, typically a bash script. This is the approach established by Carl and Daniel.

Of course, we need to try to avoid breaking existing command line interfaces. But if we can extend command line interfaces in a way that provides more flexibility or that anticipates future user needs, there are real benefits for everyone.

Ryan

— Reply to this email directly or view it on GitHub https://github.com/geodynamics/specfem3d_globe/issues/338#issuecomment-77158685.

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

komatits commented 9 years ago

Hi Ryan,

I think your idea of cleaning that part of the code was very good, so let us implement your initial idea (getting rid of hardwired region codes and adding that to the kernel name in the command line arguments). That will make the current routines much cleaner.

Thanks, Dimitri.

On 03/04/2015 07:22 PM, rmodrak wrote:

Hi Dimitri,

If it's alright, why don't we just leave the everything as it is and go ahead and close this issue.

Thanks, Ryan

— Reply to this email directly or view it on GitHub https://github.com/geodynamics/specfem3d_globe/issues/338#issuecomment-77214760.

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

rmodrak commented 9 years ago

Hi Dimitri,

I can go ahead and simplify the routines.

What about if we keep the region codes, but instead of hardwiring them in every single routine, or having to include them explicitly through a command line argument, we do something like use postprocess_par, only: reg_code?

From your previous comments, I think you would like this solution, and it would work well for me and Daniel too.

Thanks, Ryan

komatits commented 9 years ago

Hi Ryan,

You are right, very good idea.

Thanks, Dimitri.

On 03/05/2015 02:46 AM, rmodrak wrote:

Hi Dimitri,

I can go ahead and simplify the routines.

What about if we keep the region codes, but instead of hardwiring them in every single routine, or having to include them explicitly through a command line argument, we do something like |use postprocess_par, only: reg_code|?

From your previous comments, I think you would like this solution, and it would work well for me and Daniel too.

Thanks, Ryan

— Reply to this email directly or view it on GitHub https://github.com/geodynamics/specfem3d_globe/issues/338#issuecomment-77290579.

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

rmodrak commented 9 years ago

UPDATE:

I've gone ahead and addressed this issue in a new pull request #342 .

If it's alright, I'll go ahead in close this issue.