search

AI-Planning / pddl-generators

A collection of PDDL generators, some of which have been used to generate benchmarks for the International Planning Competition (IPC).
71 stars 12 forks source link

Assembly #14

Closed guicho271828 closed 2 years ago

guicho271828 commented 2 years ago

in this PR, do you prefer commits that are organized by the type of fixes, rather than by the domain?

jendrikseipp commented 2 years ago

For this PR it doesn't matter. In general, it's good to keep PRs as small as possible. But it made sense to group all compilation fixes into one PR.

guicho271828 commented 2 years ago

okay. I will also fuse this one with blocksworld one.

guicho271828 commented 2 years ago

One thing I am not sure is whether srand() will also set the seed for random() and srandom() also set the seed for rand(). I feel this is not the case and it probably needs a round of review.

guicho271828 commented 2 years ago

note that I addressed the review comments.

jendrikseipp commented 2 years ago

Regarding the random number generators: ideally we'd only use srand() and rand() since they're part of the C stdlib (https://stackoverflow.com/a/822368). And it seems the seeding can be simplified to srand(time(NULL)); (see stackoverflow link). Also, it seems including "time.h" is better than including "sys/time.h" Could you simplify the random number code in this way?

jendrikseipp commented 2 years ago

Ah, we can't seed with srand(time(NULL)); because this value only changes once per second. So we need to use the existing code which seeds by milliseconds or so. But each generator should use either rand() or random(). And ideally they should all use rand().

guicho271828 commented 2 years ago

yes, time(NULL) is too coarse. The current code uses gettimeofday and timeval.usec which should be good enough for the default behavior.

jendrikseipp commented 2 years ago

Very nice to have all these code issues fixed, thanks!