#1651: Problems with expansions in parameter files --------------------+------------------------------------------------------- Reporter: hinder | Owner: Type: defect | Status: new Priority: major | Milestone: Component: Cactus | Version: development version Keywords: | --------------------+------------------------------------------------------- If I have a parameter file containing {{{ CoordBase::ymin = -CoordBase::dy * 2 CoordBase::dy = 0.1 }}} it looks like Cactus silently ignores the CoordBase::dy in CoordBase::ymin, and tries to set ymin = 2, presumably because dy has not been set "yet". This raises two points:
1. The parser should raise a fatal error if it does not have a value for a parameter it is expanding; 2. The order of parameters listed in the parameter file should not matter.
I think that parameter files should be declarative rather than imperative; i.e. you should think of them as a static mapping from parameter names to values rather than as a program which sets (and resets?) variables in the sequence written in the parameter file. A simple way to transform the current "set each parameter in sequence" implementation would be to sort the assignments so that parameters are set before they are used, and to raise a fatal error if a parameter is set twice. Would it be possible to implement this?
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: Type: defect | Status: new Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by knarf):
I thought in previous discussions we said, that the order in this kind of expressions should matter (not that I necessarily agreed - both has advantages and disadvantages). Wasn't this so?
I agree to the error that should be raised though.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: Type: defect | Status: new Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by hinder):
I remember a discussion about this issue on a telecon, but I don't remember a conclusion. Order-dependence is the sort of thing that leads to hard-to-find bugs
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: Type: defect | Status: new Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
Technically the CoordBase::dy already has a value in the first line, namely the default value (which is 1). I am fine with making parameter files declarative rather than imperative. The required changes would seem to be:
* have the parser check before using a variable value that it has been set (ie that the Parameter_NSet or whatever call returns >= 1) * forbid $a = 42 type assignments to happen more than once for the same variable (currently $a type assignments can be changed, see )
The only legitimate use for order dependency would be something like this:
{{{ $default_dx = CoordBase::dx CoordBase::dx = 42 foo::bar = $default_dx }}}
or:
{{{ $foo = 12 bar::baza[0] = $foo bar::bazb[0] = $foo $foo = 13 bar::baza[1] = $foo bar::bazb[1] = $foo }}}
ie to either safe the default value or to set up a set of similar stanzas. Both can be achieved without though by either spelling out the default from the param.ccl files or using more variables. I tend to think that the safety benefit of declarative parfiles outweights the usefulness of these corner cases.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------ Changes (by sbrandt):
* owner: => sbrandt * status: new => accepted
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by sbrandt):
The way Piraha currently does things is that it asks Cactus for the value of the parameter, which presumably comes from some default. That doesn't prevent you from setting it a new value later and using that.
The attached patch will flag an error if a variable is written after it's read. That might give you part of what you're looking for.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by sbrandt):
This patch changes the parameter file so that variables can only be set once. Cyclic assignments and multiple assignments are now errors. This change will temporarily break the TestPar test, which violates the new rules.
In addition, this patch fixes a serious and previously undiscovered bug which caused unary operations to change the contents of the parameter they were applied to.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
I am fairly sure do not understand setonce.patch. I am not sure I understand in which respect setting a variable only once is different from disallowing parameters to change after they are being read. setonce.patch seems much more complex that write_after_read.
If both achieve the same effect (up to the unary-ops-change-their-argument issue), I'd prefer write_after_read.patch. It seems to me as if cyclical references can be detected if a parameter is marked as being "written" right when one begins to modify it and before the RHS of the assignment is actually evaluated.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by sbrandt):
The first patch does not allow you to define parameters in arbitrary order, which is what Ian wanted.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
For my money, I would actually prefer if parameters need to be defined before they are used, ie. I would prefer if {{{ a::c = a::b a::b = 12 }}} would throw an error just the same way that {{{ a::b = 12 a::b = 13 }}} throws an error. If any order is allowed then I always have to read the full parfile instead of only the top part (or hopefully just around where the reference occurs since the original assignment happened close by).
In fact {{{ a::c = a::b }}} should assign to {{{a::c}}} the default value of {{{a::b}}} as far as I am concerned. Parameters always have a value in Cactus, even when just newly created by the flesh. :-)
So I guess my preference would be for set-only-once rather than declarative and completely order independent.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by hinder):
It might be that what Roland is suggesting is better, but I would point out that requiring "set before use" might impose unnecessary structure on your parameter file. For example, you may want to put parameters into logical sections ("initial data", "evolution", "grid structure") etc, and having the requirement of "set before use" might lead to circular dependencies. I don't have an example, and I don't know if this would happen in practice, and it might indeed be easier to debug if we do it Roland's way, but I just wanted to mention this aspect here. I would be happy with a version which required "set before use" initially, and if we find it would be useful to relax the requirement, it could be changed later, without loss of backward compatibility.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by eschnett):
This is connected to activating thorns as well. I typically like to activate thorns near the section where I set their parameters. Do we allow activating thorns in any order, or do we require them to be activated in order of dependency? We should do the same for parameters -- if thorns can be activated in any order, then we should allow parameters to be set in any order as well.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by hinder):
I dislike having to activate thorns in order, probably for the same reason that I dislike having to set parameters in order. I think of a parameter file as a description of a simulation, not an ordered list of assignment statements.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by knarf):
I agree with Ian concerning thorns (dislike the current enforced order), and are "ok" with the same for parameters (although there I could see how a more complex solution might be nice - but then you can always use something Cactus-external (rpar).
The current enforcement for thorns effectively leads to a bunch of "unordered", ugly, long thorn activation lines at the beginning of par files. I would like to move them around, like Ian. I don't have a strong opinion about variables on the other hand, but I didn't really use them outside of "rpar"s. It would be confusing to have the "declaration" further "down" the file than it's usage, but it could also be useful. We could leave the decision to use or not use this feature simply to the user.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
I think allowing any order for parameter has a very similar effect as allowing parameters to be set more than once. In both cases one has to read the whole of the parfile to find out what value a given parameter has in the simulation. I would like to avoid this so that one can use a "find" in an editor to search for a parameter setting.
I rather like structured files :-).
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by sbrandt):
What I'm sensing is that my original proposal, with the write-after-read error, is the preferred solution. This surprises me, as I had the impression from the call in which this issue was first discussed that the order independent solution was preferred.
Thorn activation order no longer matters, BTW.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by hinder):
Replying to [comment:14 rhaas]:
I think allowing any order for parameter has a very similar effect as
allowing parameters to be set more than once. In both cases one has to read the whole of the parfile to find out what value a given parameter has in the simulation. I would like to avoid this so that one can use a "find" in an editor to search for a parameter setting.
I don't follow. If a parameter can be set only once, it will appear only once in a parameter file. Thus, you can use "find" to search for it, and the first (and only) occurrence will be the correct one. This is independent of whether you allow parameter expressions to contain only parameters which have been set above them in the file. Did you mean something else? Were you referring to the ability to set parameters more than once, or have them use the default value if they had not been set "yet"?
Here is another hint that we should allow any order. Suppose I have C = A followed by A = B. This would be disallowed currently, as the setting for A appears below the expression containing it. If I then remove the setting for A, magically the setting for C is now valid, as it will use the default value for A. In general, you cannot assume that you can find the value of a parameter by searching upward in the file, as the value might come from the defaults. Unless the setting is close to the use, you will probably have to search anyway, and then I don't think it makes a difference if the setting is above or below the use.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
I agree that complete order independence lets me use an editors 'find' function. My example was not a good one.
I still don't like usage patterns like this though: {{{ C = A ...lots of other stuff... A = B }}} Mostly for two reasons: 1. Cactus parameters always have values, so C = A is valid even without A = B anywhere in the file. So, to me, having to read the whole file (rather than only the part above) to find out which of the two possible values of A is going to be used is unusual. Basically since all parameters in Cactus start out having values, parfiles always '''change''' parameters rather than '''set''' them. Order independence would seem to make more sense if all assignments are required. 1. I am very commonly asked to review parameter files that are about to run (or to debug them). I start reading them from the bottom and continue to the end. If the order is completely free then for each assignment I have to search to the bottom of the file to find the possible value of the parameter. If parameters cannot be set after they have been read, I can rely on having seen any such change in the part of the file that I have just read.
Ian's example also contains magic in the case of order independence. In that case both {{{ C=A A=B }}} and {{{ C=A }}} are valid and also C would change its value from 'B' in the first case to the default of 'A' in the second. It's possible to remove the magic by requiring and "read" variable to have been "set" before. On the technical side, this needs to be an option for the expression parser so that parameters can be used in expressions that are not in parfile line righ- hand-sides (eg for the Trigger thorn or in accumulators).
However it seems to me that both Ian and I have stated out preferences, which differ and that it may be best to get some second opinion on the matter.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: accepted Priority: major | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by knarf):
For what it is worth, most examples brought up use generic variable names 'A', 'B', ect. - while in practice a reader would most often "correctly guess" from the variable name what is done here: {{{ CarpetRegrid2::radius_1[1] = CoordBase::xmax / 2 CarpetRegrid2::radius_1[1] = CarpetRegrid2::radius_1[2] / 2 }}} strongly suggests that CoordBase::xmax is set somewhere in the file, simply because that is something you would expect a user to always set. I am not arguing that it would not be possible to construct confusing examples - but rather that most cases would "intuitively understood" by the context.
I would suggest to leave the position of an assignment free, but to provide a facility in Cactus that, given a parameter file, does not run past the parameter file parser (especially does not allocate large memory), and simply outputs the parameter file with all expressions replaced. This way a review of a parameter file would be made much simpler - there would be no surprises and replacements can easily be checked even by the person sending the parameter file for review, creating even less work for the reviewer. For convenience it might even provide the "diff" between the original and processed parameter file as well.
#1651: Problems with expansions in parameter files ---------------------+------------------------------------------------------ Reporter: hinder | Owner: sbrandt Type: defect | Status: closed Priority: major | Milestone: Component: Cactus | Version: development version Resolution: fixed | Keywords: ---------------------+------------------------------------------------------ Changes (by sbrandt):
* status: accepted => closed * resolution: => fixed
Comment:
Write after read patched applied in revision 5129
trac@lists.einsteintoolkit.org
-
Einstein Toolkit