1. Einstein Toolkit Avatar Einstein Toolkit
  2. Einstein Toolkit
  3. tickets
  4. Issues

schedule.ccl SYNC allows for [timelevels] suffixes

Issue #2192 resolved
Zach Etienne created an issue

This schedule.ccl line (from an older version of GiRaFFE) should not have been permitted by the scheduler:

SYNC: GiRaFFE::grmhd_primitives_Bi, GiRaFFE::grmhd_primitives_allbutBi, GiRaFFE::em_Ax[3],GiRaFFE::em_Ay[3],GiRaFFE::em_Az[3],GiRaFFE::em_psi6phi[3],GiRaFFE::grmhd_conservatives[3],GiRaFFE::BSSN_quantities,ADMBase::metric,ADMBase::lapse,ADMBase::shift,ADMBase::curv

Keyword: piraha

Comments (28)

  2. Steven R. Brandt
    • removed comment

    Just to clarify, the correct thing is to disallow the square brackets. Correct?

  3. Roland Haas
    • removed comment

    I think so, yes. The syntax of schedule.ccl files is defined in the UserGuide (https://www.einsteintoolkit.org/usersguide/UsersGuidech12.html#x17-186000D2.4) where it says

     schedule [GROUP] <function name|group name> AT|IN <time> \
     [AS <alias>] \
     [WHILE <variable>] [IF <variable>] \
     [BEFORE|AFTER <function name>|(<function name> <function name> ...)] \
{
  [LANG: <language>]
  [OPTIONS:       <option>,<option>...]
  [TAGS:          <keyword=value>,<keyword=value>...]
  [STORAGE:       <group>[timelevels],<group>[timelevels]...]
  [READS:         <group>,<group>...]
  [WRITES:        <group>,<group>...]
  [TRIGGER:       <group>,<group>...]
  [SYNCHRONISE:   <group>,<group>...]
  [OPTIONS:       <option>,<option>...]
} "Description of function"

[...]

 <group>
    A group of grid variables. Variable groups inherited from other
thorns may be used, but they must then be fully qualified with the
implementation name.

    and a group name is defined in interface.ccl's doc (https://www.einsteintoolkit.org/usersguide/UsersGuidech12.html#x17-178000D2.2) where is says

    <data_type> <group_name>[[<number>]] [TYPE=<group_type>] [DIM=<dim>]
[TIMELEVELS=<num>]
[SIZE=<size in each direction>] [DISTRIB=<distribution_type>]
[GHOSTSIZE=<ghostsize>]
[TAGS=<string>]  ["<group_description>"]
[{
 [ <variable_name>[,]<variable_name>
   <variable_name> ]
} ["<group_description>"] ]

[...]
* group_name must be an alphanumeric name (which may also contain
underscores) which is unique across group and variable names within
the scope of the thorn.

    Meaning "forbid everything that is not either a alphanumeric string or such a string followed by :: and an alphanumeric string".

  4. Steven R. Brandt
    • removed comment

    The following patch to the flesh addresses this:

    diff --git a/src/piraha/pegs/schedule.peg b/src/piraha/pegs/schedule.peg
index e24175c..990289a 100644
--- a/src/piraha/pegs/schedule.peg
+++ b/src/piraha/pegs/schedule.peg
@@ -6,6 +6,7 @@ name = (?i:[a-zA-Z_][a-zA-Z0-9_\-]*\b)
 expr = {name}|{num}
 # TODO: Should this be a * or a ?
 vname = {name}( :: {name})*( \[ {expr} \]|)
+uname = {name}( :: {name})*
 quote = "(\\{any}|[^"])*"
 ccomment = /\*((?!\*/){-any})*\*/
 num = [+\-]?[0-9]+(\.[0-9]+)?
@@ -40,7 +41,7 @@ group = (?i:group)
 nogroup =
 prepositions = ({preposition} )*
 preposition = {par} {pararg}
-sync = (?i:sync) : {vname}( , {vname}|[ \t]{vname})*
+sync = (?i:sync) : {uname}( , {uname}|[ \t]{uname})*
 options = (?i:options?) : {vname}( , {vname}|[ \t]{vname})*
 storage = (?i:storage) : {vname}( , {vname}|[ \t]{vname})*
 triggers = (?i:triggers?) : {vname}( , {vname}|[ \t]{vname})*
  5. Roland Haas
    • removed comment

    Wouldn't this pattern uname also accept: foo::bar::baz which should not accept? I would suggest to mabye naming the pattern gname for "group" name assuming that vname stood for "variable name".

  6. Steven R. Brandt
    • removed comment

    It would. I can replace the * with ?.

    I thought there was a case where we wanted a:🅱:c. I've just checked, though, that if I change both vname and uname to use ? instead of * the ET compiles.

  7. Roland Haas
    • removed comment

    '?' is the correct modifier I agree. I had not noticed the comment already in the code about whether '?' or '*' should be used.

    I would say that when in doubt then I would start by consulting the appendices of the UserGuide which define the grammar of the ccl file and first try adhering that them. If that fails to compile the ET then either the ET has a bug (and needs fixing) or the grammar in the appendices (and in the peg files) needs to be updated b/c there was a documentation bug.

    Looking at your proposed change, I think also eg for the storage keyword one should use uname since storage is also controlled group-wide and not variable per variable.

  8. Roland Haas
    • removed comment

    Thinking about this a bit more, while indeed STORAGE takes a group name, it does accept expressions in square brackets afterwards, namely the number of timelevels for which one allocates storage. How does piraha extract those right now? Would the current patch compile and pass the test suites?

    I also notice that in the patch in line 5 the "name" pattern is

    name = (?i:[a-zA-Z_][a-zA-Z0-9_\-]*\b)

    ie it accepts "-" as a valid part of a name, which is not true for variable names (since they must be valid C identifiers) and also not really true for group names (which are alphanumeric plus the underscore).

  15. Steven R. Brandt

    Suggested patch…

    diff --git a/src/piraha/pegs/schedule.peg b/src/piraha/pegs/schedule.peg
index 977e3a68..c04d8c6a 100644
--- a/src/piraha/pegs/schedule.peg
+++ b/src/piraha/pegs/schedule.peg
@@ -2,10 +2,11 @@
 skipper = \b([\ \t\n\r\b]|{-ccomment}|\#[^\n]*|\\[\r\n])*

 any = [^]
-name = (?i:[a-zA-Z_][a-zA-Z0-9_\-]*\b)
+name = (?i:[a-zA-Z_][a-zA-Z0-9_]*\b)
 expr = {name}|{quote}|{num}
 # TODO: Should this be a * or a ?
 vname = {name}( :: {name})*( \[ {expr} \]|)
+uname = {name}( :: {name})?
 quote = "(\\{any}|[^"])*"
 ccomment = /\*((?!\*/){-any})*\*/
 num = [+\-]?[0-9]+(\.[0-9]+)?
@@ -36,9 +37,10 @@ group = (?i:group)
 nogroup =
 prepositions = ({preposition} )*
 preposition = {par} {pararg}
-sync = (?i:sync) : {vname}( , {vname}|{-spacing}{vname})*
+sync = (?i:sync) : {uname}( , {uname}|{-spacing}{uname})*
 spacing = ([ \t]|\\\r?\n)+
-options = (?i:options?) : {vname}( , {vname}|{-spacing}{vname})*
+optname = [a-zA-Z0-9-]+
+options = (?i:options?) : {optname}( , {optname}|{-spacing}{optname})*
 triggers = (?i:triggers?) : {vname}( , {vname}|{-spacing}{vname})*
 reads = (?i:reads) : {qrname}( , {qrname}|{-spacing}{qrname})*
 writes = (?i:writes) : {qrname}( , {qrname}|{-spacing}{qrname})*

    ‌

  16. Roland Haas

    The vname → uname change for sync seems fine. There is an unrelated change in the patch:

    -options = (?i:options?) : {vname}( , {vname}|{-spacing}{vname})*
+optname = [a-zA-Z0-9-]+
+options = (?i:options?) : {optname}( , {optname}|{-spacing}{optname})*

    which needs explanation I think.

    Is there maybe already a gname that is identical to uname ? For other places where groups are allowed?

  17. Steven R. Brandt

    @Roland Haas You requested that {name} not match the - character. It is needed for options, e.g. GLOBAL-EARLY. Hence, optname.

  18. Roland Haas

    ah, I see. I had not remembered that there was more hidden in this ticket than the [] in SYNC.

    Please apply.

  22. Steven R. Brandt

    @Roland Haas I don’t recall the login to the test server. Can you tell me what tests failed? Thanks.

  23. Roland Haas

    Zach changed IllinoisGRMHD so that it aborts on con2prim errors and a new test email was sent. Failing test are (the the easiest one to test is eg CarpetEvolutionMask/CarpetEvolutionMask_test):

    Baikal.magnetizedTOV-Baikal/1procs New failure
    Baikal.magnetizedTOV-Baikal/2procs New failure
    BaikalVacuum.BaikalVacuum_EE_O8_sgw3d/2procs New failure
    Carpet.kasner_amr/2procs New failure
    Carpet.test_restrict_sync/2procs New failure
    CarpetEvolutionMask.CarpetEvolutionMask_test/1procs New failure
    CarpetEvolutionMask.CarpetEvolutionMask_test/2procs New failure
    CarpetEvolutionMask.CarpetEvolutionMask_test_off/1procs New failure
    CarpetEvolutionMask.CarpetEvolutionMask_test_off/2procs New failure
    CarpetIOHDF5.CarpetWaveToyNewRecover_test_1proc/1procs New failure
    CarpetIOHDF5.CarpetWaveToyNewRecover_test_1proc/2procs New failure
    CarpetIOHDF5.CarpetWaveToyRecover_test_1proc/1procs New failure
    CarpetIOHDF5.CarpetWaveToyRecover_test_1proc/2procs New failure
    CarpetIOHDF5.CarpetWaveToyRecover_test_2proc/2procs New failure
    CarpetProlongateTest.test_cc_horest_o5/2procs New failure
    CarpetProlongateTest.test_cc_o5/2procs New failure
    CarpetProlongateTest.test_cc_tvd/1procs New failure
    CarpetProlongateTest.test_cc_tvd/2procs New failure
    CarpetProlongateTest.test_cc_tvd_hi/1procs New failure
    CarpetProlongateTest.test_cc_tvd_hi/2procs New failure
    CarpetProlongateTest.test_o11/2procs New failure
    Dissipation.test_ah/1procs New failure
    Dissipation.test_ah/2procs New failure
    Dissipation.test_ob/1procs New failure
    Dissipation.test_ob/2procs New failure
    Exact.de_Sitter/2procs New failure
    GRHydro.GRHydro_test_shock/2procs New failure
    GRHydro.GRHydro_test_shock_hllc/2procs New failure
    GRHydro.GRHydro_test_shock_mp5/2procs New failure
    GRHydro.GRHydro_test_shock_ppm/2procs New failure
    GRHydro.GRHydro_test_shock_weno/2procs New failure
    GRHydro.GRHydro_test_tov_ppm_ML/2procs New failure
    GRHydro.GRHydro_test_tov_ppm_ML_disable_internal_excision/2procs New failure
    GRHydro.GRHydro_test_tov_ppm_no_trp_ML/2procs New failure
    GRHydro.balsara1_1d/2procs New failure
    GRHydro.balsara1_1d_dc/2procs New failure
    GRHydro.balsara2_1d/2procs New failure
    GRHydro.balsara3_1d/2procs New failure
    GRHydro.balsara4_1d/2procs New failure
    GRHydro.balsara5_1d/2procs New failure
    GRHydro.cylexp_tvd_mc2_hlle/2procs New failure
    GRHydro.cylexp_tvd_mc2_hlle_dc/2procs New failure
    GRHydro.test_tov_carpet_refined_nosync/2procs New failure
    GRHydro.tov_carpetevolutionmask/2procs New failure
    GRHydro.tov_carpetevolutionmask2/2procs New failure
    GRHydro.tov_slowsector/2procs New failure
    GRHydro_InitData.magtov_poloidal/1procs New failure
    GRHydro_InitData.magtov_poloidal/2procs New failure
    GRHydro_InitData.magtov_poloidal_poloidal_P_p_2/1procs New failure
    GRHydro_InitData.magtov_poloidal_poloidal_P_p_2/2procs New failure
    GiRaFFE.GiRaFFE_tests_AlfvenWave/2procs New failure
    GiRaFFE.GiRaFFE_tests_AlignedRotator/1procs New failure
    GiRaFFE.GiRaFFE_tests_AlignedRotator/2procs New failure
    GiRaFFE.GiRaFFE_tests_DegenAlfvenWave/2procs New failure
    GiRaFFE.GiRaFFE_tests_ExactWald/1procs New failure
    GiRaFFE.GiRaFFE_tests_ExactWald/2procs New failure
    GiRaFFE.GiRaFFE_tests_FFEBreak/2procs New failure
    GiRaFFE.GiRaFFE_tests_FastWave/2procs New failure
    GiRaFFE.GiRaFFE_tests_MagnetoWald/1procs New failure
    GiRaFFE.GiRaFFE_tests_MagnetoWald/2procs New failure
    GiRaFFE.GiRaFFE_tests_SplitMonopole/1procs New failure
    GiRaFFE.GiRaFFE_tests_SplitMonopole/2procs New failure
    GiRaFFE.GiRaFFE_tests_ThreeWave/2procs New failure
    HighOrderWaveTest.test_restrict_sync/2procs New failure
    Hydro_InitExcision.diag_flip_pugh_eno/2procs New failure
    Hydro_InitExcision.diag_pugh_eno/2procs New failure
    Hydro_InitExcision.x_flip_pugh_eno/2procs New failure
    Hydro_InitExcision.x_pugh_eno/2procs New failure
    IDScalarWaveElliptic.test_waveell/2procs New failure
    IOHDF5.test_recover/2procs New failure
    IllinoisGRMHD.magnetizedTOV/1procs New failure
    IllinoisGRMHD.magnetizedTOV/2procs New failure
    LeanBSSNMoL.LeanBSSN_BY-spin/2procs New failure
    LeanBSSNMoL.LeanBSSN_BY/2procs New failure
    LeanBSSNMoL.LeanBSSN_schw/2procs New failure
    LoopControl.test-minkowski-carpet-1lev/2procs New failure
    ML_BSSN_Test.ML_BSSN_EE_sgw3d/2procs New failure
    ML_BSSN_Test.ML_BSSN_EE_sgw3d_rhs/2procs New failure
    ML_BSSN_Test.ML_BSSN_MP_O8_bh/2procs New failure
    ML_BSSN_Test.ML_BSSN_NewRad/2procs New failure
    ML_BSSN_Test.ML_BSSN_O8_sgw3d/2procs New failure
    ML_BSSN_Test.ML_BSSN_sgw3d/2procs New failure
    ML_BSSN_Test.ML_BSSN_sgw3d_harmonic/2procs New failure
    ML_BSSN_Test.ML_BSSN_sgw3d_harmonic_phi/2procs New failure
    ML_BSSN_Test.ML_BSSN_sgw3d_rhs/2procs New failure
    ML_CCZ4_Test.ML_CCZ4_sgw3d/2procs New failure
    ML_CCZ4_Test.ML_CCZ4_sgw3d_rhs/2procs New failure
    ML_WaveToy_Test.gaussian-RK4/2procs New failure
    ML_WaveToy_Test.gaussian-RK45-adaptive/2procs New failure
    ML_WaveToy_Test.gaussian-RK45/2procs New failure
    NPScalars.teukolsky/1procs New failure
    NPScalars.teukolsky/2procs New failure
    NPScalars_Proca.LeanBSSN_Ei_mu0.4_c0.05/2procs New failure
    PeriodicCarpet.ml-gw1d-small-amr/2procs New failure
    PeriodicCarpet.ml-gw1d-small/2procs New failure
    ProcaEvolve.LeanBSSN_Ei_mu0.4_c0.05/2procs New failure
    ProcaEvolve.ML_BSSN_Ei_mu0.4_c0.05/2procs New failure
    QuasiLocalMeasures.qlm-bl/2procs New failure
    RotatingSymmetry180.Kerr-EE/2procs New failure
    RotatingSymmetry180.Kerr-rotating-180-EE/2procs New failure
    RotatingSymmetry180.Kerr-rotating-180-staggered-EE/2procs New failure
    RotatingSymmetry180.Kerr-staggered-EE/2procs New failure
    RotatingSymmetry180.KerrSchild-rotating-180-EE/2procs New failure
    RotatingSymmetry90.Kerr-rotating-90-EE/2procs New failure
    RotatingSymmetry90.Kerr-rotating-90-staggered-EE/2procs New failure
    RotatingSymmetry90.KerrSchild-rotating-90-EE/2procs New failure
    SampleBoundary.wavetoyc/2procs New failure
    Seed_Magnetic_Fields_BNS.magnetized_explosionTOV/1procs New failure
    Seed_Magnetic_Fields_BNS.magnetized_explosionTOV/2procs New failure
    TerminationTrigger.walltime/1procs New failure
    TerminationTrigger.walltime/2procs New failure
    TestComplex.TestComplex/2procs New failure
    Trigger.trigger/2procs New failure
    VolumeIntegrals_GRMHD.magnetized_explosionTOV/1procs New failure
    VolumeIntegrals_GRMHD.magnetized_explosionTOV/2procs New failure
    VolumeIntegrals_vacuum.magnetized_explosionTOV/1procs New failure
    VolumeIntegrals_vacuum.magnetized_explosionTOV/2procs New failure
    WaveBinarySource.test_binary_1/2procs New failure
    WaveMoL.gaussian/2procs New failure
    WaveToy1DF77.wavetoy1df77/1procs New failure
    WaveToy1DF77.wavetoy1df77/2procs New failure
    WaveToy2DF77.test_WaveToy2D/2procs New failure
    WaveToyC.test_rad/2procs New failure
    WaveToyC.wave_output/2procs New failure
    WaveToyCXX.test_rad/2procs New failure
    WaveToyExtra.test_custom/2procs New failure
    WaveToyF77.test_rad/2procs New failure
    WaveToyF77.test_rob/2procs New failure
    WaveToyF90.test_rad/2procs New failure
    WaveToyF90.test_wavef90_flat/2procs New failure
    WaveToyF90.test_wavef90_zero/2procs New failure
    WaveToyFreeF90.test_rad/2procs New failure
    WeylScal4.teukolsky/1procs New failure
    WeylScal4.teukolsky/2procs New failure
    WeylScal4.teukolskyID/1procs New failure
    WeylScal4.teukolskyID/2procs New failure
    WeylScal4.teukolskyParity/1procs New failure
    WeylScal4.teukolskyParity/2procs New failure
    particle_tracerET.magnetized_explosionTOV/1procs New failure
    particle_tracerET.magnetized_explosionTOV/2procs New failure
    smallbPoynET.magnetized_explosionTOV/1procs New failure
    smallbPoynET.magnetized_explosionTOV/2procs New failure

  24. Roland Haas

    I am thinking it might be useful to have code in Piraha that would make it possible that all elements of the parsed grammar tree (or a subtree’s children) were “consumed” (other than the filler) to avoid accidentally introducing new peg elements but not handling them in the C/Perl code.

  29. Log in to comment
Assignee
Type
bug
Priority
minor
Status
resolved
Component
Other
Milestone
–
Version
development version
Votes
0
Watchers
0
Jira Software: the preferred issue tracker for Bitbucket. Join the team!