Prevent Nav2 AMCL from fixing srand48 seed

[hidmic]

Basic Info

Info	Please fill out this column
Ticket(s) this addresses
Primary OS tested on	Ubuntu)
Robotic platform tested on
Does this PR contain AI generated software?	No

---

Description of contribution in a few bullet points

This is not so much a bug fix as it is a conversation starter.

Basically, while AMCL does set the seed for its PRNG based on the current timestamp (here), it then overrides it with a (likely) fixed seed right after (here). This seems like a historical accident that's been around since the time Player's implementation of AMCL was first migrated to ROS.

I spotted this when I couldn't make sense out of the consistency in Nav2 AMCL's performance for a given dataset. Consistency is good, if your estimations are consistently good. It turns out seed variance translates into performance variance, and you may hit a tail of the error distribution for a fixed seed over a given trajectory. On the other hand, you may not, and allowing seeds to vary opens the door to sporadic performance degrade.

So I don't know what the right answer is. Perhaps whoever migrated this didn't either. One way or another, I think it should be explicit in documentation somewhere. It can skew comparisons with other localization systems.

---

Future work that may be required in bullet points

TBD

For Maintainers:

• Check that any new parameters added are updated in navigation.ros.org
• Check that any significant change is added to the migration guide
• Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• Check that any new functions have Doxygen added
• Check that any new features have test coverage
• Check that any new plugins is added to the plugins page
• If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

[SteveMacenski]

Interesting, pf_alloc which calls srand48(time(NULL)); seems to be called in handleMapMessage and reconfigureCB -- while pf_init is called in both of those and applyInitialPose while calling srand48(++pf_pdf_seed);

I'm curious why you chose the latter to remove instead of the former. pf_alloc is indeed called first, but please check if I'm wrong, but I don't see that pf_alloc or anything between it and pf_init actually does any sampling, that's all done in pf_init. That would point to me that we should use that instead potentially? But either way, the seed isn't casually changing mid-execution (to my reading), this is happening when soft reset events are occurring. And the seed is still changing where you point out in 2/3 conditions.

[hidmic]

By switching to Unix timestamps for PRNG seeds we know we are getting the true average performance, rather than lucking in or out depending on how that fixed seed sequence fares in a specific case. You can also reverse that argument and say that by fixing the seed sequence you ensure your system performance is more or less consistent across power cycles, rather than hoping that the next run isn't worse than what you've seen so far.

For Beluga we use the former. I don't have a strong opinion against the latter. Eventually, it would be nice to have a better understanding of how different PRN sequences affect performance for probabilistic algorithms like AMCL, but I don't have that data right now.

All that to say, I'm OK with either approach so long as it is documented somehow.

[hidmic]

CC'ing @glpuga and @nahueespinosa. I know they will find this thread interesting.

[SteveMacenski]

By switching to Unix timestamps for PRNG seeds we know we are getting the true average performance, rather than lucking in or out depending on how that fixed seed sequence fares in a specific case.

Can you defend that claim? If we're only changing seeds on new map, initial pose set, or param change, its not like that's happening all that often. For many situations, its using the first or second choice for most of the time. It is in effect far more deterministic in replaying bag files since the same dynamic parameter, new map, or initial pose events should be happening incrementing the counter for changing the seed precisely. Using the unix timestamp I would think actually creates non-determinism in playing back of the bag files since the stamp that it uses changes each time its replayed. And power cycles as you mention.

I really don't have a strong opinion about this one way or another, but it appears that removing the one that you did doesn't have the same resampling behavior for one of the soft reset events. I don't even have a strong opinion if changing the seed ever is really necessary (@mikeferguson thoughts?)

If you can show me better performance, reliably, over multiple datasets of removing one or the other (or both), I don't mind changing either/both/neither. Mostly my maintenance of AMCL is primarily focused on "don't break anything" more than it is to improve anything about it -- knowing that AMCL just needs a clean rewrite/replacement/update at some point in the future & its been "stable" for many years now (to whatever degree). Obviously there is Begula now, but haven’t had a chance yet to look into that for Nav2.

[hidmic]

I was halfway through a reply when I realized neither fixed nor time-based seeds are appropriate in all cases. For system deployment and troubleshooting, you are right, it's best to stick to a fixed seed. For performance evaluation, however, a fixed seed underestimates error variance. A sequence of Unix timestamps is just one of many sequences of seeds one could use to lessen (but not resolve) that bias.

I think the correct approach here is to make the seed a parameter. I'll rework this patch.

getting the true average performance, rather than lucking in or out

This is irrelevant outside benchmark context. I came here with a QA mindset.

If you can show me better performance, reliably, over multiple datasets of removing one or the other (or both), I don't mind changing either/both/neither.

The crux of the matter is in how you measure performance. In the limit of all possible trajectories (an infinite set), the seed choice is of no consequence (because all PRNGs are cyclic and you can always embed a trajectory in another such that it starts at any point in the cycle with a given distribution). But we can't test all possible trajectories, we test on a few. For those few, some seeds will do better, some seeds will do worse. That means I can search and find an specific seed or sequence of seeds that will work best (error distributes closer to 0) on any given dataset, but that says nothing about performance statistics over the infinite set. The same applies to the first few seeds in the current sequence (starting at 0).

My mathematical intuition says that growing the set of seeds under test will reduce the gap between sampled and true performance statistics. Thus why the seed should be a parameter.

Using the unix timestamp I would think actually creates non-determinism

It does. In hindsight, a fixed sequence of seeds would actually be a better approach to performance evaluation.

[SteveMacenski]

So... is the right path for this to regroup and take some data or to continue with a direct change now?

[hidmic]

to continue with a direct change now?

I'll submit a patch that makes the seed a parameter as soon as I find the time to do it. I'll bring some statistics with me so we can both be comfortable with the change.

[SteveMacenski]

Ok. Should we close this particular PR then since the thing to come is sometime in the nebulous future and doesn’t relate much to what’s here today?

[hidmic]

SGTM.