Carpet breaks backwards-compatibility with parameter files

Issue #616 closed
Ian Hinder created an issue

In the mercurial version of Carpet, the parameter Carpet::poison_value has been removed. This breaks any existing parameter files which use this (I have a large number). To avoid unnecessary pain, I propose that the parameter be reinstated. It can print a deprecation warning, and doesn't have to do anything.

I think we should aim to achieve backwards-compatibility with code and parameter files as much as possible, especially when it is trivial to do so. The parameter can be removed for good in a later release, if it is felt that it clutters up the code too much. I think there was a CarpetLib poison parameter which also changed, but I don't remember what it was. This should apply to that one as well.

Keyword:

Comments (6)

  1. Erik Schnetter
    • removed comment

    I suggest to add a parameter to Carpet

    CCTK_INT poison_value "UNUSED; use CarpetLib::poison_value instead" STEERABLE=always {

    • :: "" } 0

    Feel free to do so.

  2. Barry Wardell
    • removed comment

    It would probably also be good to check this parameter and print a warning if it is not its default. Maybe the default should also be something less likely to have been set in existing parameter files?

  3. Erik Schnetter
    • removed comment

    I don't think that's worthwhile. This parameter doesn't influence correctness of the evolution, so let's keep things simple. 0 is already an unlikely value, most people would use 253 or so.

    If you want to propose a patch, then it should probably run in PARAMCHECK and use CCTK_ParameterQueryTimesSet. Please test checkpointing/recovery as well to ensure the warning works as intended.

    Setting parameters which have no effect is usually not diagnosed in Cactus; this should therefore likely be a low-priority warning (maybe level 2).

  4. Barry Wardell
    • removed comment

    Replying to [comment:3 eschnett]:

    I don't think that's worthwhile. This parameter doesn't influence correctness of the evolution, so let's keep things simple. 0 is already an unlikely value, most people would use 253 or so.

    OK.

    If you want to propose a patch, then it should probably run in PARAMCHECK and use CCTK_ParameterQueryTimesSet. Please test checkpointing/recovery as well to ensure the warning works as intended.

    Setting parameters which have no effect is usually not diagnosed in Cactus; this should therefore likely be a low-priority warning (maybe level 2).

    Now that I think about it more, this seems like feature which would be better suited as a part of Cactus. It happens quite often that parameters are deprecated, so having a standardised way of denoting this in the param.ccl would be better than implementing the code manually each time.

  5. Log in to comment