guan404ming commented on code in PR #20034:
URL: https://github.com/apache/tvm/pull/20034#discussion_r3619357612
##########
web/src/support.ts:
##########
@@ -119,6 +121,29 @@ export class LinearCongruentialGenerator {
this.checkRandState();
}
+ /**
+ * Get the current generator state for deterministic restoration.
+ */
+ getState(): RNGState {
+ return this.rand_state;
+ }
+
+ /**
+ * Restore a state returned by `getState()`.
+ */
+ setState(state: RNGState): void {
+ if (!Number.isInteger(state)) {
+ throw new Error("RNG state should be an integer.");
+ }
+ if (state <= 0 || state >= this.modulus) {
+ throw new Error(
+ `RNG state should be an integer in (0, ${this.modulus}).`,
+ );
+ }
+ this.rand_state = state;
+ this.checkRandState();
Review Comment:
I think this one seems redundant, the explicit checks already guarantee the
invariant.
##########
web/src/support.ts:
##########
@@ -80,24 +80,26 @@ export function wasmPath(): string {
* Linear congruential generator for random number generating that can be
seeded.
*
* Follows the implementation of `include/tvm/support/random_engine.h`, which
follows the
- * sepcification in
https://en.cppreference.com/w/cpp/numeric/random/linear_congruential_engine.
+ * specification in
https://en.cppreference.com/w/cpp/numeric/random/linear_congruential_engine.
*
* Note `Number.MAX_SAFE_INTEGER = 2^53 - 1`, and our intermediates are
strictly less than 2^48.
*/
+export type RNGState = number;
+
export class LinearCongruentialGenerator {
readonly modulus: number;
readonly multiplier: number;
readonly increment: number;
- // Always within the range (0, 2^32 - 1) non-inclusive; if 0, will forever
generate 0.
+ // Always within the range (0, modulus) non-inclusive; if 0, will forever
generate 0.
private rand_state: number;
/**
* Set modulus, multiplier, and increment. Initialize `rand_state` according
to `Date.now()`.
*/
constructor() {
- this.modulus = 2147483647; // 2^32 - 1
- this.multiplier = 48271; // between 2^15 and 2^16
+ this.modulus = 2147483647; // 2^31 - 1
Review Comment:
Good catch, thanks!
##########
web/tests/node/test_random_generator.js:
##########
@@ -18,6 +18,23 @@
*/
const tvmjs = require("../../dist");
+function createRNGOnlyInstance() {
Review Comment:
The `Object.create(Instance.prototype)` + `empty` stub couples the test to
`uniform()` internals; the LCG-level test already covers determinism, so
comparing `rng.randomFloat()` sequences directly would be simpler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
