the flesh allows two implemantion of the same interface to have different default values for restricted parameters

Issue #745 resolved
Roland Haas repo owner created an issue

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).

Keyword:

Comments (13)

  1. Frank Löffler
    • removed comment

    Both thorns implement Driver. Shouldn't their public and restricted parameters agree in this case, including their default values?

  2. Roland Haas reporter
    • removed comment

    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).

  3. Frank Löffler
    • removed comment

    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.

  4. Roland Haas reporter
    • removed comment

    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.

  5. Erik Schnetter
    • removed comment

    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.

  6. Frank Löffler
    • removed comment

    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?

  7. Roland Haas reporter
    • changed status to open
    • removed comment

    This should do the trick. It successfully prevents me from compiling Carpet and PUGH in the same executable :-).

  8. Ian Hinder
    • removed comment

    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).

  9. Roland Haas reporter
    • removed comment
    1. 996 has been cleared. Two approvals needed since it touches the flesh?
  10. Log in to comment