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

Is *) allowed as upper boundary for a parameter range?

Issue #1595 closed
Erik Schnetter created an issue

I sometimes see these warnings when Cactus starts:

WARNING[L1,P0] (Cactus): Invalid end of real-valued parameter range; range descriptor is "(0.0 : *)" (value is 1)

Presumably the warning depends on which thorns are active.

This warning looks as if the syntax *) was accepted by the CST when parsing a param.ccl file, but was later not recognized by the flesh when checking a parameter value against this range. (The flesh uses HUGE_VAL as fallback, which happens to be correct in this case.)

Keyword: postrelease

Comments (41)

  1. Roland Haas
    • changed status to open
    • removed comment

    Likely due to a space in the corresponding param.ccl range. Perl most likely just strips the spaces when doing a regex match, but the Cactus code in Util_DoubleInRange does not allow for spaces between the ":" and the ranges. For actual numbers that does not matters since strtod strips off the leading spaces.

    Hmm, turns out that the perl code only checks ranges for integer parameters. The attached patch makes perl reject incorrectly formed CCTK_REAL ranges.

    This break all Kranc generated thorns that (incorrectly) use "*:*" (incl. the double quotes) as range.

    This breaks all thorns that use "(" or ")" as upper/lower range.

    There is also a variety of empty ranges, ranges that include a comma and square brackets (the later is actually accepted by the C code but not documented). :-)

    All of these are not allowed for integer parameters right now.

  2. Erik Schnetter reporter
    • changed status to open
    • removed comment

    Let's apply this, but make it a warning only. This should become an error after the next release, and after we've updated all parameter files and parameter file generators.

  3. Roland Haas
    • changed status to open
    • changed milestone to ET_2014_11
    • removed comment

    Applied as warning in rev 5115 of the flesh. Keeping the ticket open and setting a milestone for the next release.

  6. Erik Schnetter reporter
    • removed comment

    In a phone discussion on 2014-11-24, we decided that "*)" should be allowed. The run-time parser needs to be updated.

  9. Roland Haas
    • removed comment

    This should become an error after the next release, and after we've updated all parameter files and parameter file generators.

    Do we have param.ccl files that still fail? If not then this should become an error.

  11. Roland Haas
    • removed comment

    The committed code has a level 1 warning rather than a level 0 error in parameter_parser.pl (which is the file modified in the commit). The warning is triggered if the range does not look like: multiple stars or floats separated by colons possibly surrounded by parenthesis or quotes. The warning is currently triggered by ranges that use square brackets rather than round ones (none of which remains in the ET). Eg stuff like:

    [0:*

    and

    [0:1]
  13. Ian Hinder
    • removed comment

    I'm not sure I agree with the suggestion above to change the warning to an error. What would this achieve? We already have a warning; the only effect that I can see would be to break existing code, which I'd rather not do unless there is a tangible benefit.

  14. Steven R. Brandt
    • removed comment

    As near as I can tell, the attached trivial patch fixes the square brackets problem. I'm not sure what needs to be done to the run time. As near as I can tell, it works.

  15. Roland Haas
    • removed comment

    Fine with me (as far as I know there is no existing code that breaks since the possibility to use [ was never documented and the only thorn that I know of doing so is private to Zelmani).

    In that case we need to add documentation that documents that [ and ] can be used (as alternatives to not using any type of brackets) to specify closed ranges to the user guide. Please note that already the user guide fails to mention that ( and ) are accepted for integer type ranges to provide exclusive ranges (where it is not required since one can always adjust the allowed range by one step).

    Steve's patch actually (as far as I understand the regex) makes the the perl script explicitly accept [ which the C code will already interpret as a closed interval bound (see function Util_DoubleInRange in Misc.c).

    Allowing (* and *) (and [* and {{]) which would be equivalent to just` was already accepted in comment:6.

    We may still want to disallow using double quotes " in ranges for reals and integers. This would be similar to the changes introduced with the piraha based parser which did disallow "bare" strings (and only later re-introduced them for certain strings when it was found that a subset of users used those) and disallows quoted numbers. Allowing quotes when specifying ranges but disallowing them when specifying values in parfiles would be strange.

    The correct regex (the one in the perl script currently allows two round parens ((42:* should be:

    [(\[]?\s*($real|\*)\s*:\s*($real|\*)\s*[)\]]?

    as well as

    [\[]?$real[\]]?|[(\[]?\*[)\]]?

    to allow any combination of number and star with an optional round/square bracket at either end or a single number of single star with a round/square brackets where sensible with any whitespace in between the elements.

  16. Steven R. Brandt
    • removed comment

    So, apparently, the issue here is that we can allow three numbers in the parameter specification. Thus...

    1:10:2

    This third number, which I will call a "step", seems to mean that 1, 3, 5, 7, and 9 are allowed, but not 2, 4, 6, 8, or 10.

    I would have imagined the combination of step and bracket to look like (1:10):2 or [1:10]:2, i.e. I would have expected the brackets to go around the low and high value, and for the step to come after. Experiments indicate that the brackets always come at the end. Thus, (1:10:2) or [1:10:2].

    Based on the above, I think this regex is the one we want:

    $new_ranges =~ /^("|)[([]?(*|$real)?:(*|$real)?(:$real)?[)]]?\1$/))

    If a double quote is present at the beginning, it is required at the end. Brackets, if present, go at the start and end of the expression (but inside the quotes, obviously). A bare ":" is allowed, and it's the same as ":".

  17. Roland Haas
    • removed comment

    Good catch with the step.

    Comments: * I would disallow quotes (we disallow them for parameter values already and no one uses them, not even Kranc anymore) * do we really want to introduce ":"? There is already "" as a shorthand for ":*" * something like "1::2" would be allowed, however the parser will break on this (or at least it used to) b/c it hard-codes the sequence "::" as the separator between range and description. It should not be allowed in the range (since it currently is not). * I would not introduce a whole new plethora of options. Preferably we should do like python does: there should be one way of doing things and it should be obvious. Multiple context sensitive convenient (but only for the experts who remember them all) options is something I would like to avoid. * just to be sure: this regex cannot be the only one since it disallows single numbers or stars which are used and documented

  18. Steven R. Brandt
    • removed comment

    Quoted strings are widely used in the ET. Even making it a warning to use them will generate a ton of output.

    I wasn't introducing ":", that was already supported and used. See, for example, ADMMass, CartGrid3D, etc.

    Good point about the "::". That has to be avoided. However, it seems it was caught earlier on. The match never sees it.

    The regex doesn't need to handle a single star or number, because that's also handled by code prior to this line.

    I think we can make this an error instead of a warning. The ET passes with the above expression.

  19. Roland Haas
    • removed comment

    Quoted strings are widely used in the ET. Even making it a warning to use them will generate a ton of > output. I am not worried about quoted ''strings'' (in fact we advertise using quotes for them) I am worried about surrounding the ranges (which are a set of numbers) by quotes and we forbid surrounding numbers by quotes for parameter values, ie:

    Cactus::cctk_final_time = "10.0"

    is invalid and produces

    WARNING level 0 from host t1650-403463.aei.mpg.de process 0
  while executing schedule bin (none), routine (no thorn)::(no routine)
  in thorn Cactus, file tov.par:66:
  -> Invalid assignment: Attempting to set a variable of type REAL with (STRING)"10.0"

    I wasn't introducing ":", that was already supported and used. See, for example, ADMMass, CartGrid3D, > etc. Oh joy. Those entries are not my favorite parameter declarations :-). We will have to continue to support it (and the usage is actually backed up by the documentation it seems).

    Good point about the "::". That has to be avoided. However, it seems it was caught earlier on. The match never sees it. Well I would suspect that the range of type:

    1::2 "blah"

    gets split into a range of '1' and a description of '2 "blah"'. Eg setting

    REAL xmin "Coordinate minimum in x-direction"
{
  :: :: "Anything"
} -1.0

    gives

    CST error 1:
  -> Description of range for parameter xmin has misplaced quotes ( :: "Anything") in param.ccl for thorn CartGrid3D

    The regex doesn't need to handle a single star or number, because that's also handled by code prior to > this line. ok, just wanted to make sure.

    I think we can make this an error instead of a warning. The ET passes with the above expression. It always did (or at least has done so since we introduced the warning), Ian's concern seems to be for third party thorns.

  20. Ian Hinder
    • removed comment

    Yes; I would rather not break backward-compatibility with existing thorns if it's not necessary, just for the sake of removing a couple of lines of code. If, on the other hand, retaining the old behaviour makes the code much more complicated or is a lot of work, then we should consider it.

  21. Steven R. Brandt
    • removed comment

    Replying to [comment:19 rhaas]:

    Quoted strings are widely used in the ET. Even making it a warning to use them will generate a ton of > output. I am not worried about quoted ''strings'' (in fact we advertise using quotes for them) I am worried about surrounding the ranges (which are a set of numbers) by quotes and we forbid surrounding numbers by quotes for parameter values, ie: Cactus::cctk_final_time = "10.0" is invalid and produces WARNING level 0 from host t1650-403463.aei.mpg.de process 0 while executing schedule bin (none), routine (no thorn)::(no routine) in thorn Cactus, file tov.par:66: -> Invalid assignment: Attempting to set a variable of type REAL with (STRING)"10.0"

    Sorry Roland, I meant to say quoted ranges not quoted strings. GaugeWave, KerrSchild, ML_BSSN, ML_CCZ4, ML_WaveToy, Minkowski, etc. all use ":" repeatedly in their param.ccl files. Even producing a warning for quoted ranges creates a lot of noise (nearly 350 messages from the ET).

    I wasn't introducing ":", that was already supported and used. See, for example, ADMMass, CartGrid3D, > etc. Oh joy. Those entries are not my favorite parameter declarations :-). We will have to continue to support it (and the usage is actually backed up by the documentation it seems).

    Good point about the "::". That has to be avoided. However, it seems it was caught earlier on. The match never sees it. Well I would suspect that the range of type: 1::2 "blah" gets split into a range of '1' and a description of '2 "blah"'. Eg setting REAL xmin "Coordinate minimum in x-direction" { :: :: "Anything" } -1.0 gives CST error 1: -> Description of range for parameter xmin has misplaced quotes ( :: "Anything") in param.ccl for thorn CartGrid3D

    My experiments indicate that "1::2" gives a range starting with 1 and a step of 2.

  22. Steven R. Brandt
    • removed comment

    Replying to [comment:20 hinder]:

    Yes; I would rather not break backward-compatibility with existing thorns if it's not necessary, just for the sake of removing a couple of lines of code. If, on the other hand, retaining the old behaviour makes the code much more complicated or is a lot of work, then we should consider it.

    Yes, we could leave it as a warning. I was just pointing out that it doesn't break thorns in the ET.

  23. Barry Wardell
    • removed comment

    Replying to [comment:21 sbrandt]:

    Sorry Roland, I meant to say quoted ranges not quoted strings. GaugeWave, KerrSchild, ML_BSSN, ML_CCZ4, ML_WaveToy, Minkowski, etc. all use ":" repeatedly in their param.ccl files. Even producing a warning for quoted ranges creates a lot of noise (nearly 350 messages from the ET).

    These are all Kranc-based thorns. Would it be a good idea to modify Kranc to make it not use quoted ranges?

  26. Roland Haas
    • removed comment

    Sorry Roland, I meant to say quoted ranges not quoted strings. GaugeWave, KerrSchild, ML_BSSN, ML_CCZ4, ML_WaveToy, Minkowski, etc. all use ":" repeatedly in their param.ccl files. Even producing a warning for quoted ranges creates a lot of noise (nearly 350 messages from the ET).

    These still exist? As Frank said we had a ticket about them and I had assumed (incorrectly apparently :-) ) that this had been removed since they were all Kranc thorns (and I would expect Kranc to produce "nice" thorns).

    My experiments indicate that "1::2" gives a range starting with 1 and a step of 2. That is very strange. I just tried

    REAL xmin "Coordinate minimum in x-direction"
{
 1::2 :: "Anything"
} -1.0

    myself with 0158f72 of the flesh and it fails with this error

    CST error 1:
  -> Descrip0158f72tion of range for parameter xmin has misplaced quotes (2 :: "Anything") in param.ccl for thorn CartGrid3D

    It also fails if I try to introduce the regex like this:

    --- a/lib/sbin/parameter_parser.pl
+++ b/lib/sbin/parameter_parser.pl
@@ -396,7 +396,7 @@ sub parse_param_ccl
             if ($type eq 'REAL' && ! (
                 $new_ranges eq '*' ||
                 $new_ranges =~ /^$real$/ ||
-                $new_ranges =~ /^["(]?(\*|\(?$real)?:(\*|$real\)?)?(:$real)?[)"]?$/)) {
+                $new_ranges =~ /("|)[(\[]?(\*|$real)?:(\*|$real)?(:$real)?[)\]]?\1$/)) {
               # this will become a level 0 error in the fture
               &CST_error(1, "Invalid range '$new_ranges' for real " .
                          "parameter '$variable' of thorn '$thorn'. ".

    Whatever we decide to do in the end (I don't really care very much what we do) we need to make sure that the C code parses the range string (unfortunately it is passed the string, the perl code does not pass the result of its own parsing to C) in the very same way.

    The only reason ":" works is that Cactus removes all quotes from ranges when creating the parameter C code in line 534 of lib/sbin/CreateParameterBindings.pl even though that line 381 of parameter_parser.pl would seem to only allow quotes for string parameters (also note that integer parameters never allowed ":" or any quotes around the range for that matter so we have some work to do to make this all consistent between parameter types).

  29. Roland Haas
    • changed status to open
    • removed comment
    • documentation is missing for [ to indicate closed intervals
    • documentation in synatx of a range entry is relagated to appendix
    • the docs say that a description of a range is optional, yet this triggers a warning when used
    • the documentation does not mention the use of a stride for reals
    • the documentation says one can use C like postfixes "f" and "l" for float and long double values, however the $real regex in parameter_parser.pl will not allow this (but does incorrectly accept Fortran style D instead of E for the exponent.
    • parameter_parser.pl will not accept [ or ( for INT paramters yet the C code does
    • the documentation (and the C code) indicate that "1::2" should indeed be acceptable ranges, yet the parser aborts since it takes the first :: to separate range and description.
  30. Erik Schnetter reporter
    • removed comment

    My suggestions: - disallow open/closed specifications for integer ranges (they are not necessary, and are confusing) - disallow strides for real ranges (they cannot be implemented correctly due to round-off, except for integer multiples of powers of 2) - disallow 1::2, since we don't allow omitting range endpoints in any other case either - disallow f and l suffixes, since the parser doesn't handle them, and we interpret all numbers as CCTK_REAL and CCTK_INT anyway - continue to accept d for exponents for backward compatibility without warning, but advise against it in the documentation

  31. Roland Haas
    • removed comment

    • disallow open/closed specifications for integer ranges (they are not necessary, and are confusing) Fine with me.

    • disallow strides for real ranges (they cannot be implemented correctly due to round-off, except for integer multiples of powers of 2) Fine with me in principle, yet Ian's comments on possibly breaking things for no good reason apply since right now one CAN use these options, they are just not documented (same as quotes around ranges)

    • disallow 1::2, since we don't allow omitting range endpoints in any other case either They are already not usable since an earlier part of the Perl parser breaks. They are however documented as an option in particular the docs in UserGuide.pdf section C1.4.1 explicitly say that "A missing end of range (or a ‘*’) implies negative or positive infinity." so this is actually a bug. Independent of this it would be good to fix the parser so that it can handle "::" in the range eg for string type parameters where "::" appears if one wants to construct a fully qualified grid variable or parameter name (currently we work around by usin "THORN[:][:]PARAMETER").

    • disallow f and l suffixes, since the parser doesn't handle them, and we interpret all numbers as CCTK_REAL and CCTK_INT anyway Fine with me. Needs update to the docs.

    • continue to accept d for exponents for backward compatibility without warning, but advise against it in the documentation This never worked since the C code always just passed the string to strtod which does not handle "d". This bug was introduced in a2c2335 by me. Right now only parameter ''values'' in par files can use "dD".

  33. Steven R. Brandt
    • removed comment

    Frank asked me to look at this, as the ticket is still open. I plan to update the docs and then close the ticket. I think if there are to be new changes we should open a new ticket. Objections?

  35. Roland Haas
    • removed comment

    Could you tell is how you intend to update the docs? Since this amounts to more than just a documentation update but actually setting the allowed formats for parameter ranges, it would be good if this was at least looked at once before being committed. This is mostly based on a desire to have the documentation as the definitive description of the the parameter file syntax and not the combined perl/C code that handles the files.

  36. Steven R. Brandt
    • removed comment

    So, this ticket has become slightly confusing as the bug it is targeting seems to keep changing with time. After I last marked it as fixed, you (Rhaas) re-opened it citing specific things that weren't in the documentation (along with some other things), e.g. "documentation is missing for [ to indicate closed intervals".

    My feeling is that if we want other fixes, e.g. "the documentation says one can use C like postfixes "f" and "l" for float and long double values, however the $real regex in parameter_parser.pl will not allow this (but does incorrectly accept Fortran style D instead of E for the exponent" there should be a new ticket for that issue.

  38. Frank Löffler
    • removed comment

    After reading the few last comments: Steve: Did you update the docs as you said you plan to? If so, please close this ticket. If there is something else wrong with the system or docs concerning ranges ect., please open a new ticket.

  42. Log in to comment
Assignee
Type
bug
Priority
minor
Status
closed
Component
Cactus
Milestone
ET_2016_05
Version
development version
Votes
0
Watchers
0
Jira Software: the preferred issue tracker for Bitbucket. Join the team!