schedule.ccl SYNC allows for [timelevels] suffixes

Create issue
Issue #2192 open
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 (11)

  1. Steven R. Brandt
    • removed comment

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

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

  3. 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})*
    
  4. 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".

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

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

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

  8. Log in to comment