#271 [generate] Add count

[mzuenni]

tried to implement count... its a bit hacky. Fixes #271

[RagnarGrootKoerkamp]

Hmm this makes a test fail. Didn't check yet but maybe the seeding subtly changed? Would be best to avoid that if we can.

Also this will need some tests of its own and docs.

[mzuenni]

maybe it would be nice to be able to use count "inline" i.e. combining it with this: - name: gen {seed} instead of needing to write this:

- name:
    generator: gen {seed}
    count: 5

[RagnarGrootKoerkamp]

Hmm, maybe something like {seed:count=5}? But that's already abused for changing the seed value.

Or completely replace {seed} by {count=N}?

On the other hand this is already a shorthand to generate multiple cases; spelling them out a bit more verbosely to clearly signal that this creates multiple cases may not be so bad?

[mzuenni]

beeing explicit with count is good it is just unusual to write the generate key as its the default else :D

[RagnarGrootKoerkamp]

Fair point. One option that we shouldn't do is

- name: <gen>
  count: cnt

but that is super ugly and also doesn't work for non-numbered cases.

[mzuenni]

that also doesn't work with the yaml parser if i remember correctly

[mpsijm]

Can you also add this key to the generators.yaml in the documentation folder, and perhaps also in the commented-out part of the skeleton? :slightly_smiling_face:

[mzuenni]

can someone update the cue to include count?

[thorehusfeldt]

can someone update the cue to include count?

Yes; maybe this is more of a regex question than a cue question.

Here’s the current regex for command:

// A command invokes a generator, like "tree --n 5".
// The regex restricts occurrences of curly-bracketed expressions
// to things like "tree --random --seed {seed:5}"
command: !="" & (=~"^[^{}]*(\\{seed(:[0-9]+)?\\}[^{}]*)*$")

The important part is the regex. It disallows curly-brackets except for {seed} and possibly stuff like {seed:2316}.

To make the new, better, regex, I’d need some examples of what is allowed (and what is forbidden) with {count}.

While we’re here: does {name} still exist? (Current CUE regex forbids it, as you can see, so it appears nowhere in the repo yaml.) It is now always "testcase", right?

Edit: Maybe what I write above misses the mark completely, and all you did was add the key

count: int &  >= 0 // or maybe >0 ?

to #testcase? That’s even easier to fix. Just tell me what to do.

[RagnarGrootKoerkamp]

The syntax is as here https://github.com/RagnarGrootKoerkamp/BAPCtools/pull/353#issuecomment-1981027148 Just a new count keyword alongside generator, with a value between 1 and 100 inclusive.

[thorehusfeldt]

By “alongside generator” do you mean a new key for a #testcase struct/map/dict, alongside generate, such as

testcase_name:
  generate: foo --bar 2 {seed}
  count: 23

If so, is the following allowed:

testcase_name:
  in: "34 5"
  count: 23

or can count only appear in a #testcase when generate occurs?

[thorehusfeldt]

I added my best interpretation of the syntax, but remain confused. In doc/generators.yaml we have

      12-counted:
          - generate: graph {seed:3}
          - count: 2                       # generate two testcases at once

Is this just wrong? (My schema complains; maybe that’s a good thing.)

[mzuenni]

copy is only valid if there is generate: <command> entry where command contains {seed} or {seed:[0-9]+} (not sure if its possible to check the seed).

The entry looks good to me? (the indention is also right or?) never mind there should be no minus XD

[thorehusfeldt]

CUE actually allows

    generate?: string
    if generate != _|_ {
        if generate =~ "{seed(:[0-9]+)?}" {
          count?: int & >= 1 & <= 100
        }
    }

which is exactly the rule “make count available if generate is available and conforms to this-and-that regex”. But I decide against this; let’s have a readable but not maximally invasive schema. (CUE syntax for conditional optional fields will change on this point anyway.)

[thorehusfeldt]

… now we just need to get 351, 353 (which both manipulate the schema) merged… I’ll stand way back and cover my ears.

[thorehusfeldt]

Another TODO is to update the JSON schema at schemastore. That’s a much more painful proces. I’ll wait with that until the dust settles; you have to live with your favourite editor complaining about count fields.

[RagnarGrootKoerkamp]

So currently the cue allows count even if there is no generate:. Fine by me.

@mzuenni How does generate.py handle this? Does it already warn if count is set alongside in: (or anything else than generate:)?

Thinking about it, there are in fact usecases for count: alongside in:, for example when the interactor internally generates a random seed and we just need 100 empty input files. So we should either support that or warn that it's not handled.

[mzuenni]

We generate a parse error if count is used without a generator with seed. Yes, there are use cases for this, however, we currently forbid identical rules anyway and count is no exception to this as it just duplicates the rule.

If you want I can lift that restriction for count by including count_index in the hash for the cache entry. We then could also propose count as a solution if we find identical rules instead of just skipping those duplicates?

[mzuenni]

Actually my prefered way right now would be this:

• I fix the parallel issue for duplicated testcases (shouldn't be hard)
• We still warn for duplicate testcases
• We say that if you want to suppress that error you should use count.
• We allow count for all kinds

[RagnarGrootKoerkamp]

I was gonna write that we can postpone generalizing count until we need it, but that plan sounds nice, so go ahead :)

(We could still merge this already if you feel like it.)

[mzuenni]

Shouldn't be too complicated :)

[mzuenni]

I think right now everything works as expected?

[RagnarGrootKoerkamp]

Can you add some count rules to the identity test problem? Also for both generate: and in: and copy: rules