#1595: Is {{{*)}}} allowed as upper boundary for a parameter range? ----------------------+----------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: development version Keywords: | ----------------------+----------------------------------------------------- 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.)
#1595: Is {{{*)}}} allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by rhaas):
* status: new => review
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.
#1595: Is {{{*)}}} allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reviewed_ok Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by eschnett):
* status: review => reviewed_ok
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.
#1595: Is {{{*)}}} allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: minor | Milestone: ET_2014_11 Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by rhaas):
* status: reviewed_ok => reopened * milestone: => ET_2014_11
Comment:
Applied as warning in rev 5115 of the flesh. Keeping the ticket open and setting a milestone for the next release.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: minor | Milestone: ET_2014_11 Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+----------------------------------------------------
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+---------------------------------------------------- Changes (by knarf):
* keywords: => postrelease * milestone: ET_2014_11 =>
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by eschnett):
In a phone discussion on 2014-11-24, we decided that "*)" should be allowed. The run-time parser needs to be updated.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+---------------------------------------------------- Changes (by knarf):
* owner: => sbrandt * status: reopened => assigned
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by hinder):
I have created https://github.com/ianhinder/Kranc/issues/109 for the issue mentioned above that Kranc generates invalid (but currently-accepted) param.ccl files.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by rhaas):
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+---------------------------------------------------- Changes (by rhaas):
* milestone: => ET_2015_05
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by rhaas):
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] }}}
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by knarf):
Steve: ping
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by hinder):
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by sbrandt):
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by rhaas):
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by sbrandt):
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 "*:*".
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by rhaas):
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
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by sbrandt):
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by 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" }}}
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by 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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by sbrandt):
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 parameter 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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by sbrandt):
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by barry.wardell):
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?
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by knarf):
It would, and there is even a ticket about it: https://github.com/ianhinder/Kranc/issues/109
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by sbrandt):
Do people want to see warnings for quoted ranges then?
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by rhaas):
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).
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: assigned Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by eschnett):
{{{1::2}} is not valid; this should be {{{1:*:2}}}, or {{{1:*):2}}}.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: closed Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: fixed | Keywords: postrelease -----------------------+---------------------------------------------------- Changes (by sbrandt):
* status: assigned => closed * resolution: => fixed
Comment:
Committed fix to the flesh.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+---------------------------------------------------- Changes (by rhaas):
* status: closed => reopened * resolution: fixed =>
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by eschnett):
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
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2015_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by rhaas):
- 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".
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2015_11 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+---------------------------------------------------- Changes (by knarf):
* milestone: ET_2015_05 => ET_2015_11
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2015_11 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by sbrandt):
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?
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2015_11 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by knarf):
Sounds good to me, please do.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2015_11 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by rhaas):
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2015_11 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by sbrandt):
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+---------------------------------------------------- Changes (by knarf):
* milestone: ET_2015_11 => ET_2016_05
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by knarf):
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.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: reopened Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: development version Resolution: | Keywords: postrelease -----------------------+----------------------------------------------------
Comment (by sbrandt):
I pushed a fix for the docs to a0ef065b2d05a6ab6e59fd700daf7a24f12c53fb. It only impacts section C.1.4.1.
#1595: Is *) allowed as upper boundary for a parameter range? -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: sbrandt Type: defect | Status: closed Priority: minor | Milestone: ET_2016_05 Component: Cactus | Version: development version Resolution: fixed | Keywords: postrelease -----------------------+---------------------------------------------------- Changes (by knarf):
* status: reopened => closed * resolution: => fixed
Comment:
Please open a new ticket for similar issues.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit