Add WVUThorns_Diagnostics to the ET

Issue #1974 closed
Zach Etienne created an issue

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:

Comments (20)

  1. Roland Haas
    • removed comment

    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.

  2. Zach Etienne reporter
    • removed comment

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

  3. Zach Etienne reporter
    • edited description

    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. "

  4. 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

  5. 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/.

  6. 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.

  7. Zach Etienne reporter

    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.

  8. 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).

  9. Log in to comment