#2192: schedule.ccl SYNC allows for [timelevels] suffixes ---------------------------------+-------------------- Reporter: zachetie@… | Type: defect Status: new | Priority: unset Milestone: | Component: Other Version: development version | Keywords: ---------------------------------+-------------------- 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
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+--------------------------------- Changes (by Roland Haas):
* keywords: => piraha * owner: (none) => Steven R. Brandt * status: new => assigned
Comment:
This looks like a failure in the piraha parser to me.
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+---------------------------------
Comment (by Steven R. Brandt):
Just to clarify, the correct thing is to disallow the square brackets. Correct?
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+---------------------------------
Comment (by Roland Haas):
I think so, yes. The syntax of schedule.ccl files is defined in the UserGuide (https://www.einsteintoolkit.org/usersguide/UsersGuidech12.html#x17-186000D2....) 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....) 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".
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+---------------------------------
Comment (by Steven R. Brandt):
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})* }}}
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+---------------------------------
Comment (by Roland Haas):
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".
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+---------------------------------
Comment (by Steven R. Brandt):
It would. I can replace the * with ?.
I thought there was a case where we wanted a::b::c. I've just checked, though, that if I change both vname and uname to use ? instead of * the ET compiles.
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+---------------------------------
Comment (by Roland Haas):
'?' 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.
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+--------------------------------- Changes (by Steven R. Brandt):
* Attachment "vname.patch" added.
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+---------------------------------
Comment (by Roland Haas):
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).
#2192: schedule.ccl SYNC allows for [timelevels] suffixes -------------------------+--------------------------------- Reporter: zachetie@… | Owner: Steven R. Brandt Type: defect | Status: assigned Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: piraha -------------------------+---------------------------------
Comment (by Steven R. Brandt):
No, the current patch doesn't pass. I'm working on it.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit