Add WVUThorns_Diagnostics to the ET

[Zach Etienne]

I have open-sourced a very useful set of diagnostic routines for GRMHD, GRHD, and even vacuum simulations.

The thorns are available here: https://bitbucket.org/zach_etienne/wvuthorns_diagnostics

Here's a brief description of each thorn: Brief description of included thorns

'''Seed_Magnetic_Fields-modified''' Extended Seed_Magnetic_Fields thorn for binary neutron stars. Supercedes Seed_Magnetic_Fields thorn.

'''Meudon_Bin_NS-modified''' Modifications to Meudon BNS initial data thorn to disable the overwriting of initial lapse/shift, which acts to significantly reduce coordinate eccentricity. Supercedes Meudon_Bin_NS thorn.

'''VolumeIntegrals_GRMHD''' Nice GRMHD volume integration thorn, currently depends on IllinoisGRMHD and Carpet. Performs volume integrals on arbitrary "Swiss-cheese"-like topologies, and even interoperates with Carpet to track NS centers of mass.

'''VolumeIntegrals_vacuum''' Nice GRMHD volume integration thorn, currently depends on ML_BSSN. There is a bit of code duplication and duplicated functionality between VI_GRMHD and VI_vacuum, to ensure that VI_vacuum can be used without enabling a GRMHD code. There is probably a better way of doing this, but I haven't had the time to think deeply about this.

'''particle_tracerET''' Solves the ODE \partial_t x^i = v^i for typically thousands of tracer particles, using an RK4 integration atop the current timestepping. E.g., one RK4 substep in the particle integration might occur every 16 RK4 substeps in the GRMHD evolution. These tracer particle positions are quite useful for visualizing magnetic field lines in a consistent way from frame-to-frame in a movie (recall that in the GRMHD approximation, the magnetic field lines stay attached the fluid elements they thread ["Flux Freezing"]). Note that the velocity must be consistent with the velocity appearing in the GRMHD induction equation. This thorn reads in the HydroBase vel[] vector gridfunction, which assumes the Valencia formalism, and converts it into the induction equation velocity.

'''smallbPoynET''' Computes b^i^, b^2^, and three spatial components of Poynting flux. It also computes (-1-u,,0,,), which is useful for tracking unbound matter.

Keyword:

[Frank Löffler]

ping

[Frank Löffler]

Meudon_Bin_NS-modified

Can you see in how far the changes described in https://trac.einsteintoolkit.org/ticket/2039 maybe do the same as what you intended here?

[Roland Haas]

If these are to go into ET_2019_02 (March release), who is reviewing them? They should also be added to the master thorn list as soon as feasible to test them on some clusters.

[Zach Etienne]

Steve has agreed to review them. Tests are in the process of being added.

[Roland Haas]

thank you. I have assigned the ticket to Steve.

[Steven R. Brandt]

This arrangement has about 2k lines of code. Now that tests are available, I'm in favor of adding it.

One problem I have with the code is that it makes use of the c-preprocessor when C++ offers better ways of doing things. I've created a pull request with a suggested refactoring: https://bitbucket.org/zach_etienne/wvuthorns_diagnostics/pull-requests/1/avoid-using-the-c-preprocessor-to-define/diff

[Zach Etienne]

I approve inclusion of this arrangement (specifically, git commit ID d01b02d of the wvuthorns_diagnostics git repo, hosted on Bitbucket.org) into the March 2019 ET release.

Steve Brandt replied in email to Roland Haas, Peter Diener, & Zach Etienne on March 21, 2019:

" I approve also. "

[Roland Haas]

I am getting link time errors from the use of number_of_reductions(int) on Blue Waters using the gnu compiler. The error message is:

/mnt/a/u/staff/rhaas/ET_Proca/arrangements/WVUThorns_Diagnostics/VolumeIntegrals_vacuum/src/file_output_routines.C:179: undefined reference to `number_of_reductions(int)'

and this code was introduced in 049ceff (adding the inline and replacing some, but unfortunately not all, instances of its used by #include "number_of_reductions.C", triggering the error) and in the initial upload.

This is /b the function is defined in number_of_reductions.C as inline however the C++ standard says (https://en.cppreference.com/w/c/language/inline):

If a non-static function is declared inline, then it must be defined in the same translation unit. The inline definition that does not use extern is not externally visible and does not prevent other translation units from defining the same function.

where the key phrase is "same translation unit", which basically means "same call to the compiler".

The function would be incorrectly named for a function with external linkage since all thorn functions that are externally visible must be either in a namespace named after the thorn or have a prefix that includes (or is at least based on) the thorn name: https://www.einsteintoolkit.org/usersguide/UsersGuidech9.html#x13-139000C1.9.7

[Zach Etienne]

Hi Roland,

I'm working to reproduce this now on BW using GNU GCC. When GetComponents-ing the latest dev version of ETK, a number of errors appeared, resulting in the ExternalLibraries not checking out. For example:

Could not checkout module ExternalLibraries/libjpeg svn: OPTIONS of ' https://github.com/EinsteinToolkit/ExternalLibraries-libjpeg.git/trunk': SSL handshake failed: SSL error: tlsv1 alert protocol version ( https://github.com)

[Roland Haas]

Since this just happened to me: when trying to reproduce this, it is best to do a make sim-clean before (or at least remove configs/sim/build/VolumeIntegrals_vacuum) to make sure that there is no old file number_of_reconstructions.C present in configs/sim/build/VolumeIntegrals_vacuum that would be picked by #include "number_of_reconstructions.C" before it wold look at the one in arrangements/WVUThorns_Diagnostics/VolumeIntegrals_vacuum/src/.

[Roland Haas]

To further add to the confusion, for C code (but the affected code in VolumeIntegrals_vacuum is C++), we seem to rely on the old pre-C99 behaviour of inline which would make inline appear externally as well (and then should complain about multiply defined symbols). See https://bitbucket.org/einsteintoolkit/tickets/issues/484/cctk_check_c_inline-can-define-away-the . The safest solution is most likely to always declare an inline functions as static inline which agrees between old and new usage.

[Roland Haas]

Plus gcc itself changed how it handles inline (in gcc 5): https://www.gnu.org/software/gcc/gcc-5/porting_to.html

[Steven R. Brandt]

I had the mistaken belief that "inline" was equivalent to "static inline." I've update the repo (https://bitbucket.org/stevenrbrandt/wvuthorns_diagnostics) to use static inline.

[Zach Etienne]

Hi Steve,

I beat you to it. The latest master fixes the problem. Please take a look at the latest commit if you like. It passes all tests.

[Roland Haas]

@stevenrbrandt I think you are correct: in C99 and in C++11 (since it models itself after C99) "inline" is very close to static inline in that by default no external linkage is visible. However, for historical reasons I assume, Cactus wanted inline to behave in the pre-C99 (GNU) way and we test whether or not this is the case in autoconf and the redefine the inline keyword.

You C++ code seems to have worked on all the systems where the compiler is old enough that by default the C inline statement follows the GNU convention (ie gcc < 5.0).

My current guess why this failed on some systems and worked on others is that on those machines autoconf decided not to change anything which left C++ inline alone (since no inline was redefined) and it thus defaulted to its default behaviour which is not like GNU-C (but like C99). However on new machines autoconf detects that C's default is not what it wants and redefines inline for both C++ and C.

At least this would be my best guess. One would have to inspect cctk_Config.h to see what was actually done.

Zach changes made things work again so all is fine for now. If possible I will try and make a pull request for after the release to have Cactus accept C99's default inline behaviour (since we require C99 anyway).