Add randomesque#46
Conversation
|
I looked at it again and compared it to the original catR implementation. I realised there were some differences, especially in ties handling. I adapted my implementation to mirror catR better. Sorry for missing that earlier. |
richford
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @jakub-jedrusiak. The overall approach looks good, and extending it to selectorClosest (catR-style) was the right call. With randomesque = 1 as the default, this is fully backwards-compatible and the existing mfi / closest tests pass without modification. A few items below before I can approve.
Also, the "Usage" section in README.md lists the Cat constructor options (method, itemSelect, nStartItems, theta, ...) but randomesque is missing. Could you:
- Add
randomesqueto one of the examplenew CAT({...})calls. - Add a one-line description in the surrounding prose.
- Optionally, mention in the Validation section ("Validation of MFI algorithm — Reference software: catR") that
randomesquefollows catR's behavior.
| this._theta = theta; | ||
| this._seMeasurement = Number.MAX_VALUE; | ||
| this.nStartItems = nStartItems; | ||
| this.randomesque = Math.max(1, Math.round(randomesque)); |
There was a problem hiding this comment.
This quietly turns 0, -3, 2.7, NaN, etc. into something "valid," which is inconsistent with how every other CatInput option is handled, e.g., validateMethod, validateItemSelect, validateStartSelect, and validatePrior all throw on bad input. Could you add a Cat.validateRandomesque static (or inline check) that throws on non-positive integers and non-numbers? Something like:
private static validateRandomesque(randomesque: number): number {
if (!Number.isInteger(randomesque) || randomesque < 1) {
throw new Error(
`randomesque must be a positive integer. Received ${randomesque}.`,
);
}
return randomesque;
}| })); | ||
|
|
||
| if (stimuliAddFisher.length === 0) { | ||
| return { nextStimulus: undefined as unknown as Stimulus, remainingStimuli: [] as Stimulus[] }; |
There was a problem hiding this comment.
The double-cast hides a real type contract. findNextItem already returned undefined for nextStimulus in the empty-stimuli case before this PR (the test at cat.test.ts:286-287 relies on it), so the actual return type has always been { nextStimulus: Stimulus | undefined; remainingStimuli: Stimulus[] }. The new explicit guards are an improvement, but the casts paper over a pre-existing latent typing bug. Concrete fix:
- Declare a return-type alias. Add after
CatInputcloses atcat.ts:22:
export interface SelectorResult {
nextStimulus: Stimulus | undefined;
remainingStimuli: Stimulus[];
}- Annotate
findNextItem.cat.ts:257, change:
public findNextItem(stimuli: Stimulus[], itemSelect: string = this.itemSelect, deepCopy = true) {to:
public findNextItem(
stimuli: Stimulus[],
itemSelect: string = this.itemSelect,
deepCopy = true,
): SelectorResult {-
Annotate the private selectors. Same
: SelectorResultannotation onselectorMFI(cat.ts:292),selectorClosest(cat.ts:340),selectorMiddle(cat.ts:324),selectorRandom(cat.ts:363), andselectorFixed(cat.ts:381). The latter three already implicitly returnStimulus | undefinedfornextStimulus(e.g.arr.shift()on an empty array,arr[index]whenindexis out of bounds), so the annotation just makes the existing contract explicit. -
Drop the casts in
selectorMFI.cat.ts:299-301, replace:
if (stimuliAddFisher.length === 0) {
return { nextStimulus: undefined as unknown as Stimulus, remainingStimuli: [] as Stimulus[] };
}with:
if (stimuliAddFisher.length === 0) {
return { nextStimulus: undefined, remainingStimuli: [] };
}5.. Drop the casts in selectorClosest. cat.ts:342-344, same change:
if (arr.length === 0) {
return { nextStimulus: undefined, remainingStimuli: [] };
}- Drop the cast on
picked.**cat.ts:319, change:
nextStimulus: picked as Stimulus,to:
nextStimulus: picked,The as Stimulus was only there to satisfy the previous (incorrect) return shape; once SelectorResult.nextStimulus is Stimulus | undefined, picked (which is Stimulus) widens cleanly.
- Update the JSDoc.
cat.ts:255, change:
@returns {nextStimulus: Stimulus, remainingStimuli: Array<Stimulus>}
to:
@returns {nextStimulus: Stimulus | undefined, remainingStimuli: Array<Stimulus>}
After these changes, tsc --noEmit should be clean and the call site in clowder.ts:410 (const { nextStimulus } = cat.findNextItem(...)) will correctly type nextStimulus as Stimulus | undefined, which matches how Clowder already treats it downstream.
| const nextItem = arr[index]; | ||
| arr.splice(index, 1); | ||
| if (arr.length === 0) { | ||
| return { nextStimulus: undefined as unknown as Stimulus, remainingStimuli: [] as Stimulus[] }; |
There was a problem hiding this comment.
Same comment here about the double-cast.
| const nrIt = Math.min(this.randomesque, distances.length); | ||
| const threshold = distances[nrIt - 1].dist; | ||
| const topK = distances.filter((d) => d.dist <= threshold); | ||
| const pick = topK[this.randomInteger(0, topK.length - 1)]; |
There was a problem hiding this comment.
selectorMFI and selectorClosest now have the same threshold/filter/randomInteger pattern. Could you extract a small helper, e.g.:
private pickFromTopK<T>(sorted: T[], score: (item: T) => number): T {
const nrIt = Math.min(this.randomesque, sorted.length);
const threshold = score(sorted[nrIt - 1]);
const topK = sorted.filter((s) => score(s) >= threshold);
// For closest, score returns *distance*, so the comparator flips.
// pass an `order: "desc" | "asc"` flag, or have the caller pre-negate.
return topK[this.randomInteger(0, topK.length - 1)];
}If we ever revisit the tie semantics it'll only need to change in one place.
|
|
||
| const nrIt = Math.min(this.randomesque, distances.length); | ||
| const threshold = distances[nrIt - 1].dist; | ||
| const topK = distances.filter((d) => d.dist <= threshold); |
There was a problem hiding this comment.
The >= threshold / <= threshold filters mean that when items tie at the K-th rank, all of them are included in the random pool. E.g., with randomesque = 2 and Fisher info [5, 3, 3, 3, 1], the pool is the three 3s, not two of them. I think this is intentional. Does catR do the same thing? If it's intentional, please add a comment explaining it to the reader so that the next developer doesn't try to come in and "fix" it.
| * priorDist: the prior distribution type (only applies to EAP estimator) | ||
| * priorPar: the prior distribution parameters (only applies to EAP estimator) | ||
| * randomSeed: set a random seed to trace the simulation | ||
| * randomesque: number of top items to randomly select from in MFI/closest (default = 1, i.e. always pick the best) |
There was a problem hiding this comment.
| * randomesque: number of top items to randomly select from in MFI/closest (default = 1, i.e. always pick the best) | |
| * randomesque: when selecting an item via 'MFI' or 'closest', randomly pick from the | |
| top N candidates rather than always taking the best. Use values > 1 to introduce | |
| controlled variability and reduce item exposure across examinees, at a small cost | |
| to measurement efficiency. Items tied at the N-th rank are all included in the | |
| random pool, so the effective pool size may exceed `randomesque` when ties exist. | |
| This mirrors catR's `randomesque` behavior. Must be a | |
| positive integer; throws if invalid. Default = 1 (always pick the best). |
| * @param stimuli - an array of stimulus | ||
| * @param itemSelect - the item selection method | ||
| * @param deepCopy - default deepCopy = true | ||
| * @returns {nextStimulus: Stimulus, remainingStimuli: Array<Stimulus>} |
There was a problem hiding this comment.
With the new empty-array guards, nextStimulus can now be undefined.
| * @returns {nextStimulus: Stimulus | undefined, remainingStimuli: Array<Stimulus>} |
| * @param {{method: string, itemSelect: string, nStartItems: number, startSelect:string, theta: number, minTheta: number, maxTheta: number, priorDist: string, priorPar: number[]}=} destructuredParam | ||
| * @param {{method: string, itemSelect: string, nStartItems: number, startSelect:string, theta: number, minTheta: number, maxTheta: number, priorDist: string, priorPar: number[], randomSeed: string|null, randomesque: number}=} destructuredParam | ||
| * method: ability estimator, e.g. MLE or EAP, default = 'MLE' | ||
| * itemSelect: the method of item selection, e.g. "MFI", "random", "closest", default method = 'MFI' |
There was a problem hiding this comment.
| * itemSelect: the method of item selection, e.g. "MFI", "random", "closest", default method = 'MFI' | |
| * itemSelect: the method of item selection, e.g. "MFI", "random", "closest", default method = 'MFI'. "MFI" and "closest" additionally honor the `randomesque` parameter. |
| } | ||
| } | ||
|
|
||
| private selectorMFI(inputStimuli: Stimulus[]) { |
There was a problem hiding this comment.
Even though this is a private method, I think it's important enough to have a JSDoc string. selectorFixed already has one. This was a pre-existing gap, but now that both methods have non-trivial algorithmic behavior (top-K + tie inclusion + RNG), this is the right moment to add them.
| /** | |
| * Picks the item with the highest Fisher information at the current theta | |
| * estimate. When `randomesque > 1`, randomly picks from the top-K candidates | |
| * by Fisher information rather than always taking the best, which trades a | |
| * small amount of measurement efficiency for reduced item exposure across | |
| * examinees. Items tied at the K-th rank are all included in the random pool, | |
| * so the effective pool size may exceed `randomesque` when ties exist (this | |
| * mirrors catR's behavior; see Magis & Barrada, 2017). | |
| * | |
| * The temporary `fisherInformation` field used internally for ranking is | |
| * stripped from both the returned stimulus and the remaining stimuli before | |
| * returning. | |
| * | |
| * @param inputStimuli - The list of stimuli to choose from. | |
| * @returns {Object} - An object with the next item and the updated list. | |
| * @returns {Stimulus | undefined} return.nextStimulus - The picked item, or | |
| * `undefined` if `inputStimuli` is empty. | |
| * @returns {Stimulus[]} return.remainingStimuli - The remaining stimuli, sorted | |
| * by difficulty ascending. | |
| */ | |
| private selectorMFI(inputStimuli: Stimulus[]) { |
| @@ -319,9 +339,21 @@ | |||
|
|
|||
| private selectorClosest(arr: Stimulus[]) { | |||
There was a problem hiding this comment.
Same here
| private selectorClosest(arr: Stimulus[]) { | |
| /** | |
| * Picks the item whose difficulty is closest to a target value derived from | |
| * the current ability estimate (see the note on the `0.481` offset below). | |
| * When `randomesque > 1`, randomly picks from the K nearest items rather than | |
| * always the closest. Items tied at the K-th-nearest distance are all included | |
| * in the random pool, so the effective pool size may exceed `randomesque` when | |
| * ties exist (this matches catR's `randomesque` semantics in the `bOpt` rule). | |
| * | |
| * @param arr - The list of stimuli to choose from. | |
| * @returns {Object} - An object with the next item and the updated list. | |
| * @returns {Stimulus | undefined} return.nextStimulus - The picked item, or | |
| * `undefined` if `arr` is empty. | |
| * @returns {Stimulus[]} return.remainingStimuli - The remaining stimuli, in | |
| * the order they had on input (with the picked item removed). | |
| */ | |
| private selectorClosest(arr: Stimulus[]) { |
One of the strategies for dealing with determinism in CATs is to select one of N best fitting items instead of the best item every time. This ensures the tests are not necessarily the same given a specific input and prevents overuse. It is implemented in catR as "randomesque". I needed it for my current work so I made this PR. It adds
randomesqueto theCatclass and modifies the selectors to utilise it. Whenrandomesque = 1(default), it works as always but specifying e.g.,randomesque = 5, the item selection will include 5 items with e.g. highest information and will randomly select one of them.Citation
Stocking, M. L., & Lewis, C. (2000). Methods of controlling the exposure of items in computerized adaptive testing. In W. J. van der Linden & C. A. W. Glas (Eds.), Computerized adaptive testing: Theory and practice (pp. 163–182). Kluwer Academic Publishers.