search

Kattis / problem-package-format

Kattis problem package format specification
https://www.kattis.com/problem-package-format
9 stars 14 forks source link

time_multiplier(s)? #349

Open mzuenni opened 1 week ago

mzuenni commented 1 week ago

I am a little late to the party but i have a few questions regarding time_multiplier(s).

  1. is ac_to_time_limit exactly the same as the legacy time_multiplier
  2. is time_limit_to_tle exactly the same as the legacy time_safety_margin
  3. if yes, just why?

Now assuming that these are indeed the same I can see that the new names are easier to understand and that the new defaults might be more reasonable but i don't think that that is a sufficient justification. I want to argue that this is not a good idea and we should undo the changes:

As a tool writer:

As a Judge:

To clarify this a bit more: Not only do i now live in a world where i can never be sure what default values are used (because tooling decides if this is legacy or not and some tooling will take more time to implement the new format) also random unrelated changes could somehow influence if the problem is detected as legacy or not and therefore change the default values.

For example it could happen that adding a uuid suddenly changes this from a legacy to a new problem and therefore changing the timelimit... So i can only be safe if i actually specify the new keys even if i want the new default values.

niemela commented 1 week ago

I am a little late to the party but i have a few questions regarding time_multiplier(s).

Maybe a little late, but not too late šŸ˜„.

  1. is ac_to_time_limit exactly the same as the legacy time_multiplier

Yes.

  1. is time_limit_to_tle exactly the same as the legacy time_safety_margin

Yes.

  1. if yes, just why?

Mainly that we had gotten some feedback that the original names were hard to understand.

Now assuming that these are indeed the same I can see that the new names are easier to understand

:+1:

and that the new defaults might be more reasonable

I think very much so.

but i don't think that that is a sufficient justification. I want to argue that this is not a good idea and we should undo the changes:

As a tool writer:

  • I have to write more code to support this without getting any new features

True, but it's very little more code.

  • I have to properly figure out if the problem is legacy or not because the default values changed

Which is very easy because the name also changed.

The old default value was a very old regret of mine, I think the old defaults (esp. time_multiplier) are quite terrible (there are some explanations to why we "had" to make that choice though, in short, compromises where made šŸ˜„). The fact that we changed the name is what "allowed" us to fix that now.

As a Judge:

  • I now have to write more to get the same results (why are the new keys under time_multipliers ?)

Again, it's very little, but true.

  • I have to always specify them since i cant rely on some tooling somehow figuring out if this problem is legacy or not

This I disagree with. If you're being strict, problem is legacy if problem_format_version is legacy or missing, it is the new version if problem_format_version exists and says so. This is very easy to check. If you want to be a bit more sloppy it would be reasonable to assume the default for time_multiplier is 5, and ac_to_time_limit is 2.

To clarify this a bit more: Not only do i now live in a world where i can never be sure what default values are used (because tooling decides if this is legacy or not and some tooling will take more time to implement the new format) also random unrelated changes could somehow influence if the problem is detected as legacy or not and therefore change the default values.

What kind of "random changes"? Again, see above how to strictly determine if a package is legacy.

For example it could happen that adding a uuid suddenly changes this from a legacy to a new problem and therefore changing the timelimit...

Absolutely not! uuid is allowed (and actively used) in legacy.

So i can only be safe if i actually specify the new keys even if i want the new default values.

I disagree with this claim for the reasons above.

Note that I'm only disagreeing with some of your points. I can still very much see the general point that the benefits (easier to understand names and better defaults) are not worth the cost (having to learn the new names, longer names and more updates needed to tooling). I don't think the cost is very high, but I agree that the benefits are not huge either.

I'm definitely open to revert. What do others think?

mzuenni commented 1 week ago

Ok, so just to clarify my main point here: even though it is properly defined when which multipliers are used my problem is that it is confusing that things with the same meaning but different names and default values exists for different versions... (which i still think will end up in a weird mixture that people and tooling will (try) to handle somehow)

Anyway, my preferred change would be to revert this (and just keep the new explanation). This makes stuff more backwards compatible and i think neither the old names nor the old default values are that bad (sure the new ones would be better but that is not enough for a change). For most NWERC and GCPC problems any setting of the parameters would result in a timelimit of 1sec so even though the old ones are not optimal they are good enough in many cases.

Even if there is a consensus for the new names I would still want to keep rid of the time_multipliers map and just put the new entries in limits directly. Not only is time_multipliers really close to time_multiplier which adds to the confusion, i also don't see any advantage of this, its just more stuff to type?

Tagl commented 1 week ago

I very much agree with our change of the default values. The new defaults are much more common values. Aside from the really easy problems, I almost always change the time_multiplier from 5 to 3 or lower because there is a Python/Java/C#/Prolog submission under accepted.

I don't think the name change was necessary but I see where the complaints came from. The description of the key would still always clarify what it means. So on that point I can go either way.

Since version is changing alongside it, there is no compatibility issue really. I don't think there is much of a cost to this change since it is also really easily and automatically updatable from old version to new version.

evouga commented 1 week ago

I would be sympathetic to the argument that the cost is high for little gain if this were an isolated, minor revision to the spec.

But that's not the case: this draft is a major revision with many changes all over the place. People will already need to consciously familiarize themselves with the changes and update their problem package templates. We might as well take the opportunity to make the package format easier to use and to fix old mistakes---it's now or "never" (in many years, whenever the next major revision takes place.)

As I see it, there will be some short term pain that will quickly be forgotten (as people copy-paste example problem templates that have all of the new names in them), in exchange for long-term gain (less misuse of the time multipliers due to confusion about what they mean, and due to naive use of inappropriate default values).

eldering commented 1 week ago

Agreed with above points made by @Tagl and @evouga .

It would be nice if {bapc,problem}tools would detect such legacy keys and error and suggest the new alternatives. OTOH, the translation part can be deferred to a migration tool (that we still need to create).

eldering commented 1 week ago

Maybe to add: I'd be ok with renaming the nested

time_multipliers
    ac_to_time_limit: 2.0
    time_limit_to_tle: 1.5

if we have a clearly better alternative.

But just to give our reasoning for this nesting (which we discussed quite a bit):