[Einstein Toolkit] #745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters --------------------+------------------------------------------------------- Reporter: rhaas | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Keywords: | --------------------+------------------------------------------------------- since the flesh creates paramters for all compiled in (rather than activated) thorns initially, this affects the default value that a thorn sees. Thorns seem to be initialized (and thus their parameter structures being created) in alphabetical order, which means that the thorn alphabetically '''last''' (How, I have no idea, the parameter handling logic seems a bit of a mess) will determine the default parameter value.
Attached is a parameter file to demonstrate this with Carpet and PUGH who both declare parameters periodic and periodic_[xyz] but differ in their defaults.
To demonstrate, create an executable with only Carpet compiled in and run the parameter file. Then look at the paramters in the checkpoint it creates eg. {{{ h5dump -r -d /Parameters\ and\ Global\ Attributes/All\ Parameters output/checkpoint.chkpt.it_0.h5 | grep periodic }}} Do the same with an executable that contains both PUGH and Carpet. Notice that parameter values are now PUGH's defaults.
This can actually cause a runs to abort when recovering from a checkpoint when one switches from an executable with PUGH compiled in to one that does not. (Beyond the fact that some thorn might actually use it's parameters rather than the Carpet/PUGH pair where happily Carpet ignores these parameters and PUGH who actually used them gets to set the default).
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by knarf):
Both thorns implement Driver. Shouldn't their public and restricted parameters agree in this case, including their default values?
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
yes, they should. They do not however. From PUGH/param.ccl: {{{ BOOLEAN periodic_x "Periodic boundary conditions in x-direction" { } "yes" }}}
From Carpet/param.ccl: {{{ BOOLEAN periodic_x "do not use this parameter" { } "no" }}}
the choice for Carpet is understandable: no reason to set the default value for a paramter to something that we don't support anyway (periodic boundaries). Also the test in Carpet's ParamCheck routines makes sense in this respect since it tests if the paramter has been changed (but not its actual value).
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by knarf):
So there are two problems: the first that Cactus does not enforce an identical API (which it should), and that Carpet and PUGH disagree - which would have to be discussed and solved.
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
Yes that is certainly a better way of stating the issue. Should I open up a second ticket for Carpet and PUGH? The contents would be almost the same as this ticket this one and I would use the same example parameter file.
Note: a check for identical default values at runtime is (almost) trivial to do in CCTKi_ParamterCreate, the only messy piece being that one has to normalize the default values (ie. "yes" for all boolean true values etc). Ideally this would happen at configuration time though I think. I expect that Cactus already has most information at hand at configuration time, since --- I believe --- I get errors if two thorns with the same implementation do not declare the same (restricted) parameters.
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by eschnett):
You can change Carpet's defaults there; Carpet ignores the periodicity parameters.
Yes, the check should occur in the CST.
Generally, I wonder why such a check is necessary, since only one thorn will be active. Cactus has no business using disabled thorns to set default values for parameters. Maybe we do want to allow different default values? A bug in Cactus shouldn't make us insert another check to avoid this bug.
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by knarf):
I would expect the default values to be part of the API which defines an implementation. Otherwise replacing an implementation using the same paramter file (which does not set that paramter) would lead to most likely undesired behavior.
Should we take this discussion to the users mailing list?
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------ Changes (by rhaas):
* status: new => review
Comment:
This should do the trick. It successfully prevents me from compiling Carpet and PUGH in the same executable :-).
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by hinder):
In case it needs to be said: please don't apply this until the parameters are made consistent so that you can compile them together (#996).
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
#996 has been cleared. Two approvals needed since it touches the flesh?
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by eschnett):
Approved.
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: reviewed_ok Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------ Changes (by eschnett):
* status: review => reviewed_ok
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: reviewed_ok Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: ---------------------+------------------------------------------------------
Comment (by rhaas):
ok to apply?
#745: the flesh allows two implemantion of the same interface to have different default values for restricted parameters ---------------------+------------------------------------------------------ Reporter: rhaas | Owner: Type: defect | Status: closed Priority: minor | Milestone: Component: Cactus | Version: Resolution: fixed | Keywords: ---------------------+------------------------------------------------------ Changes (by rhaas):
* status: reviewed_ok => closed * resolution: => fixed
Comment:
Applied as rev 4865 of the flesh.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit