per-variable tolerances for Cactus testsuites

Issue #114 closed
Frank Löffler created an issue

It would be nice to be able to specify per-variable testsuite tolerances in Cactus (per regexp for the name in the ideal case).

Keyword:

Comments (25)

  1. Tanja Bode
    • changed status to open
    • removed comment

    I've attached a patch to RunTestUtils.pl which implements this feature. Both absolute and relative tolerances can be specified with the keywords ''VarExpABSTOL'' or ''VarExpRELTOL'' followed by a regular expression to match (without the leading m/ or trailing /) and the new tolerance value. The specified tolerances override more general tolerances for data files whose name matches these regular expressions. Any set of characters can be used for matching as long as there are no whitespaces in the regular expression.

    For example:

    1. !div style="font-size: 80%" #!python VarExpABSTOL ^Psi4.[xy] 1e-8 VarExpRELTOL gxx 1e-12

    More specific tolerances can be specified for all the tests of a thorn or just within a test's block.

    The attached patch passed the tests I gave it. Please review.

  2. Ian Hinder
    • removed comment

    Thanks for the patch. I have the following comments:

    • There is some debugging code left in, some of it commented out. Probably this should be removed.
    • The new keywords, VarExpABSTOL and VarExpRELTOL don't fit with the all-caps conventions of other keywords accepted here.
    • From looking at the code, it looks like you can only specify each of these once. This means that you can only specify a variable-specific tolerance for the variables whose names match one regular expression, and only one tolerance value. Unfortunately, this might not be enough in most of the cases where this would be used. Is this correct?

    Maybe a better interface would be to use the existing ABSTOL and RELTOL keywords, but to allow two arguments. The first would be the tolerance, and the second would be a regular expression determining which variables to apply this tolerance to. It would default to '.*'. Additionally, the mechanism would be extended so that more than one ABSTOL or RELTOL can be provided so that different variable patterns can have different tolerances.

  3. Tanja Bode
    • removed comment

    Thanks for looking at the patch.

    • Concerning debugging code, the only debugging left uncommented is in PrintDataBase, which is in there specifically ''for'' debugging and only for debugging. This should be left in there so that the database printing routine descends one step lower in the hierarchy, giving useful information instead of just the reference to the hash. There is a PrintDataBase call in RunTest.pl that is commented out. If we don't want any commented debugging, this should also be deleted. If we want no debugging at all committed, that entire function should be removed.
    • I fully expected a complaint concerning the keywords. Easily changed. Long all-caps really annoy me so I prefer not to use it while coding. Search and replace are sufficient. I apologize for having left the case is is. Figured there would be enough discussion concerning functionality that there would be changes anyway. I didn't think of a better choice for the keyword, though, that wasn't absurdly long or misleading.
    • I implemented it as an extra keyword as it was easier to parse the lines this way, and specifying the expression first and tolerance second makes more sense to me as a user. Changing how ABSTOL and RELTOL are specified would outdate all current test.ccl files and would be a much more invasive change. I could put in extra loops of logic to treat both old and new cases. I should think on this some more, whether there is a good way to handle this, but I prefer a second keyword.

    The interface in the patch ''does'' allow any number of expressions to be specified at either level. Where it falls short is in logic when a file matches more than one expression. Currently any test tolerances override thorn-wide tolerances, and expression-specific tolerances blindly override all other tolerances. If it matches more than one expression, though, it's only the last match's tolerance that is used. How should we treat cases where a filename matches more than one expression? I'm inclining towards throwing an error if this happens indicating that test.ccl should be modified.

  4. Ian Hinder
    • removed comment

    OK, in that case, the debugging code should be left in. I didn't look too closely at that, it was just a general observation.

    I hadn't considered the case of files matching more than one expression. As you say, in that case probably the code should abort and the user should make the expressions more specific. If this becomes a problem in future, we can relax the restriction and print a warning that only the last expression is being used.

    The reason I said that the code wouldn't work for multiple regexps is that I had misunderstood the code on line 219. Sorry. You are correct, it should work fine.

    I disagree that my suggestion would invalidate all current test files. My proposed interface includes the original interface as a special case - i.e. the user specifies ABSTOL = 1e-9. They can optionally put a regexp after it if they want to use the new feature. I agree that the opposite order is a bit more logical, but that would mean that you couldn't interpret this as an additional optional positional argument which defaulted to '.*'. I think that the new behaviour is just an extension of the old, so a new keyword is unnecessary. It also means that you need separate code to handle the two cases, whereas if you have the same keyword, you can use the new code exclusively, with just one additional line to say "the default regexp is '.*'". This reduces code duplication (you can then remove the current ABSTOL and RELTOL cases in the code). Unless I have missed something else!

  5. Tanja Bode
    • removed comment

    This proved simpler than expected. The latest attachment now upgrades the ABSTOL and RELTOL entries to hashes. If a test's datafile matches more than one expression aside from the default at the given level (thorn-wide or test-specific), a warning is thrown and the script is verbosely aborted.

    New usage requires the format

    ABSTOL <Tolerance> [Expression] RELTOL <Tolerance> [Expression] where [Expression] defaults to .* if missing.

    Random examples:

    ABSTOL 1e-5 ABSTOL 1e-10 Psi[0-9] ABSTOL 1e-12 g[xyz] ABSTOL 1e-4 (.*)*.x.asc RELTOL 1e-12 .*

    Again, any number of expressions are allowed at any level as long as a file matches one and only one extra tolerance expression.

    This passes every test I've thrown at it and it fits everything that was desired, so please review it.

  6. Frank Löffler reporter
    • removed comment

    The patch looks good, but I disagree with the restriction of only one matching expression for each of them. One of the probably most used cases is going to be "apply the tolerance A to all files except files matching expression X where tolerance B should be applied, and possibly a few others like C, D and E. With the current mechanism I would have to build an expression which matches !A&!B&!C&!D&!E, which quickly gets hard to read and maintain.

    How difficult would it be to have either the first or the last matching entry count? (I have a slight preference for the first, would don't care much; both can be argued for and against.)

  7. Tanja Bode
    • removed comment

    It would not be difficult at all to take only the first and last instance, however I disagree that uniqueness requires unreadable expressions. You might have to break it up into several lines that are more specific to address subsets of the larger group. In the extreme case you would simply enter the entire file name. It's certainly possible to make it both unique and readable with a minimal familiarity of regular expressions.

    The problem with taking the first or last instance is if the thorn developer adds an expression which matches more than was intended, increasing the tolerance of a secondary data file to the point where it's no longer a good test. There's no verboseness option available in test.ccl to developers. Any warnings put into the RunTest scripts to alert the developer would warn everybody, every time they run the test.

    If you insist going with first or last instance, then I would also insist we include an option to print a tolerance table for a test, where the tolerances for every available datafile in the test are tabulated. That way a developer can assess the success of their expression set without having to modify the RunTest*.pl perl scripts. For some tests this would apparently require 200+ lines .... might not actually be a bad addition either way actually.

  8. Tanja Bode
    • removed comment

    The latest patch moves the tolerance logic into an extra function for readability and avoids doubled code. It also adds an option to the menu which prints a tolerance table for the last run test. To avoid a huge table, this includes the effective global tolerances for that test and then only those files where a tolerance differs from these.

    The question still remains whether to allow the file to match more than one expression and how to handle that case. I still maintain that requiring a unique expression may mean more lines in test.ccl, but it is safer and not necessarily less readable.

  9. Frank Löffler reporter
    • removed comment

    I agree that an option to print the tolerances used would be useful.

    I don't understand your argument with the entire file name though. The problematic case I see is when you want to set the tolerance for all files to some value, but the tolerance of some other groups to other tolerances. The other files should not be a problem, but the regex for "all others" in this case cannot be .* because that would match everything. It would need to contain essentially all other regexes negated - which would also be error prone if someone changes one but forgets about the corresponding negation.

    For example

    ABSTOL 1e-5 ABSTOL 1e-10 Psi[0-9] would not be allowed with the current patch, because the `Psi[0-9]` files also match the first line. In this case you would have to write a regex which specifies 'everything but something matching `Psi[0-9]`. And now imagine something more complex like

    ABSTOL 1e-10 Psi[0-9] ABSTOL 1e-12 g[xyz] ABSTOL 1e-4 (.*)*.x.asc ABSTOL 1e-13 .* Certainly all files of numbers 1-3 match also number 4, but files matching 2 might also match 3. Getting this group exclusive would make it hard to read.

  10. Roland Haas
    • removed comment

    Quick question: does the current patch make sure that

    ABSTOL 1e-12 g[xyz] does not match a file

    arrgghh-gx-i-did-not-want-to-match-this file Maybe anchoring at least the beginning to something like

    (^|(::)) (I want to say: beginning of string or thornname::varname) would make sense. Even we only do so ourself by hand in the testsuite test.ccl files we distribute.

  11. Frank Löffler reporter
    • removed comment

    I think it would match it. It a real test.ccl file I would expect the anchor to be present. I would not enforce it though, e.g. it would make more sense to write (for files not containing the thorn name):

    ABSTOL 1e-10 ^g[xyz]{2}_.*\.asc$

  12. Tanja Bode
    • removed comment

    Ah, I had not made clear in my comments that the case .* alone is treated specially to specify a global tolerance at that (thorn/test) level. It is not counted when considering how many extra regular expressions have matched.

    As to the anchor, the expression in the test.ccl file should be the entire regular expression sans slashes: /$expr/ and as such if you want the expression to be anchored, it must be specified there. I would not want to place a ''hidden'' anchor to be added to the expression as then the expression in the test.ccl file is misleading, especially if the developer does not want an anchor.

    The current patch that I'm about to upload takes only the ''first'' match (beyond the default .* case). The developer is required to place any anchor in her regular expression. The new menu item (P) prints a tolerance table for the last test run so a developer can quickly see which tolerance was used for which file. (The first point is the only thing I just now changed).

  13. Roland Haas
    • removed comment

    I looked at the patches again. The very latest patch has an issue where it seems to pick the second matching (non-'.*') pattern instead of the first one. Another thing that I found confusing is that the order of the regex (ie. which is first, second etc) is determined by sorting the regular expressions alphabetically (not eg. the order in which the patterns appear in the file).

    Tanja promised an updated patch for the first issue. For the second: this seems to be completely avoidable if we only allow a single (non-'.*') match which should not be too hard to construct. If one really ever needs something like:

    g[xyz][xyz]\..\.asc 1e-9 g[xyz][xyz]\.d\.asc 1e-8 Then one can simply change the first pattern to `g[xyz][xyz]\.[xyz]\.asc`. This might yield a number of warnings if a new pattern is added that conflicts with existing ones but that seems preferable to having a new pattern silently overwriting existing ones. The uniqueness requirement means that it is sufficient to find the single matching pattern when creating new entries rather than having to understand all patterns to deduce their applicability. Implementing an order based on the order of which the patterns appear in test.ccl is annoyingly complicated (unless one uses a third party module Tie::IxHash which Tanja found) since hashes in Perl do not preserve the order in which entries were added. So for now I would suggest we go with Tanja's initial implementation that only allows a single match (beyond '.*') and then if and when we encounter a situation like then one Frank described in comment 8 someone can go back and add this option without breaking backwards compatibility. It would seem good to me to have some implementation ready well before the next release so that we have time to test it.

    So: please apply (the patch looks good) with the second vs. first issue fixed and maybe even with the original uniqueness requirement.

  14. Frank Löffler reporter
    • removed comment

    Tanja - can you please provide the promised fix so that we can apply and close this?

  15. Roland Haas
    • removed comment

    Patch looks good (always did). I copied Tanja's description in the application thorn guide. Please apply both patches.

  16. Eloisa Bentivegna

    A very belated thanks for adding this feature!

    I would like to report two shortcomings in the documentation though. First, the example given in documentation.patch should be:

    \begin{alltt} 
        ABSTOL 1e-8 ^Psi4.[xy] 
        RELTOL 1e-12 gxx 
    \end{alltt}
    

    rather than:

    \begin{alltt} 
        ABSTOL ^Psi4.[xy] 1e-8 
        RELTOL gxx 1e-12 
    \end{alltt}
    

    Second, the statement that:

    It is an error if a regular expression matches more than one filename.
    

    had me a little confused at first. Do I understand it right from the above discussion, that it is an error when a filename matches more than one regular expression, not the other way around?

  17. Roland Haas
    • changed status to resolved
    • removed comment

    Elo: Yes, it is an error if a single filename is matched by more than one regular expression. Ie if you have files ab.asc and ac.asc and a test.ccl

      ABSTOL a.[.]asc 17
      ABSTOL ab[.]asc 42
    

    then this is an error. We could have said "the first one that matches is used" but this seemed dangerous. So right now the script actually finds all matching regexs and aborts if more than one matches. A can match multiple filenames (would not make much sense otherwise), ie .

      ABSTOL a.[.]asc 17
    

    is fine and matches both ab.asc and ac.asc.

  18. Log in to comment