UpOpt math code in gi/upopt library #3069
Conversation
…s (domains, strongbox, elixir, reshaping)
WalkthroughIntroduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant upOpt as upOpt.ts
participant expandSub as expandSubstat
participant expandRolls as expandRolls
participant subProbs as substatProbs
participant dedup as deduplicate
participant eval as evaluation
User->>upOpt: levelUpArtifact(artifact, build)
upOpt->>subProbs: crawlSubstats(prefix, options)
subProbs-->>upOpt: [{p, subs}, ...]
upOpt->>expandSub: makeSubstatNode(info)
expandSub-->>upOpt: SubstatLevelNode
loop For each SubstatLevelNode
upOpt->>expandSub: expandSubstatLevel(node)
expandSub-->>upOpt: [{p, n: RollsLevelNode}, ...]
upOpt->>expandRolls: expandRollsLevel(rollsNode)
expandRolls-->>upOpt: [{p, n: ValuesLevelNode}, ...]
end
upOpt->>dedup: deduplicate(nodes)
dedup-->>upOpt: [deduplicated nodes]
upOpt->>eval: evalMarkovNode(obj, node)
eval-->>upOpt: EvaluatedMarkovNode[]
upOpt-->>User: [evaluated, weighted nodes]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: This introduces a substantial, multi-layered probabilistic optimization framework spanning 20+ files across configuration, type system, mathematical utilities, and algorithm modules. The core logic density is high—roll distribution expansion, Gaussian covariance propagation, Markov node deduplication, and statistical re-weighting each demand careful verification. While individual algorithms follow recognizable patterns (combinatorial crawling, matrix operations, node traversal), their composition and correctness guarantees require understanding the entire pipeline. The comprehensive test suite aids review but heterogeneous mathematical logic, multiple node types, and edge cases (reshaping constraints, zero-derivative pruning, probability normalization) across files elevate complexity. Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[zzz-frontend] [Sat Sep 27 14:29:18 UTC 2025] - Deployed e4ef3ba to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Sep 27 14:29:27 UTC 2025] - Deployed e4ef3ba to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Sep 27 14:29:50 UTC 2025] - Deployed e4ef3ba to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Sep 27 14:30:36 UTC 2025] - Deployed 9acb58d to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Sep 27 14:30:49 UTC 2025] - Deployed 9acb58d to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Sep 27 14:30:59 UTC 2025] - Deployed 9acb58d to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Sep 27 17:09:39 UTC 2025] - Deployed 2975e53 to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Sep 27 17:09:49 UTC 2025] - Deployed 2975e53 to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Sep 27 17:10:11 UTC 2025] - Deployed 2975e53 to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Sep 27 17:14:41 UTC 2025] - Deployed 61c0fb2 to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Sep 27 17:14:58 UTC 2025] - Deployed 61c0fb2 to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Sep 27 17:16:59 UTC 2025] - Deployed 61c0fb2 to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Sep 27 17:37:54 UTC 2025] - Deployed 3a2fc6b to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Sep 27 17:38:48 UTC 2025] - Deployed 3a2fc6b to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Sep 27 17:39:03 UTC 2025] - Deployed 3a2fc6b to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Sep 27 17:50:58 UTC 2025] - Deployed d46ab0e to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Sep 27 17:51:38 UTC 2025] - Deployed d46ab0e to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Sep 27 17:51:53 UTC 2025] - Deployed d46ab0e to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Sep 27 18:07:19 UTC 2025] - Deployed 658ba30 to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Sep 27 18:07:35 UTC 2025] - Deployed 658ba30 to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Sep 27 18:07:39 UTC 2025] - Deployed 658ba30 to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Sep 27 18:10:33 UTC 2025] - Deployed 25d72bd to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Sep 27 18:10:37 UTC 2025] - Deployed 25d72bd to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Sep 27 18:10:43 UTC 2025] - Deployed 25d72bd to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Sep 27 19:20:25 UTC 2025] - Deployed 35c69d1 to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat Sep 27 19:20:34 UTC 2025] - Deployed 35c69d1 to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat Sep 27 19:20:38 UTC 2025] - Deployed 35c69d1 to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Tue Sep 30 05:58:13 UTC 2025] - Deployed 06ee7ba to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Tue Sep 30 05:58:30 UTC 2025] - Deployed 06ee7ba to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Sep 30 05:58:36 UTC 2025] - Deployed 06ee7ba to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Oct 20 16:58:29 UTC 2025] - Deployed 1f2d75d to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 20 16:59:05 UTC 2025] - Deployed 1f2d75d to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Mon Oct 20 17:01:46 UTC 2025] - Deployed 1f2d75d to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Mon Oct 20 17:03:30 UTC 2025] - Deployed a380135 to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 20 17:03:56 UTC 2025] - Deployed a380135 to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Oct 20 17:04:13 UTC 2025] - Deployed a380135 to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Oct 27 19:51:52 UTC 2025] - Deployed b912c64 to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Mon Oct 27 19:52:04 UTC 2025] - Deployed b912c64 to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 27 19:52:23 UTC 2025] - Deployed b912c64 to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Mon Oct 27 19:55:02 UTC 2025] - Deployed 36eff67 to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Oct 27 19:55:14 UTC 2025] - Deployed 36eff67 to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 27 19:55:26 UTC 2025] - Deployed 36eff67 to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Mon Oct 27 19:56:07 UTC 2025] - Deployed 8aa24e2 to https://genshin-optimizer-prs.github.io/pr/3069/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Oct 27 19:56:16 UTC 2025] - Deployed 8aa24e2 to https://genshin-optimizer-prs.github.io/pr/3069/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Oct 27 19:57:51 UTC 2025] - Deployed 8aa24e2 to https://genshin-optimizer-prs.github.io/pr/3069/sr-frontend (Takes 3-5 minutes after this completes to be available) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
libs/gi/upopt/src/mathUtil.test.ts (1)
53-56: Prefer a non-mutating numeric sort for the key.It's 3 AM, the banner is dry, and seeing in-place lexical sort makes my eyelid twitch—let’s keep the original tuple untouched and make the ordering explicit.
- const strKey = ns.sort().join('') + const strKey = [...ns].sort((a, b) => a - b).join('')libs/gi/upopt/src/makeObjective.ts (1)
44-46: Reuse the cached nonzero-derivative lookup.Running
zero_derivagain for every substat is like rolling a 10-pull after hitting pity—unnecessary pain. We already havenonzeroDerivs, so lean on it instead.- zeroDeriv: allSubstatKeys.filter((s) => - nodes.every((n) => zero_deriv(n, (f) => f.path[1], s)) - ), + zeroDeriv: allSubstatKeys.filter((s) => + nonzeroDerivs.every((subs) => !subs.includes(s)) + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
libs/gi/upopt/.eslintrc.json(1 hunks)libs/gi/upopt/project.json(1 hunks)libs/gi/upopt/src/consts.ts(1 hunks)libs/gi/upopt/src/deduplicate.ts(1 hunks)libs/gi/upopt/src/evaluation.ts(1 hunks)libs/gi/upopt/src/expandRolls.ts(1 hunks)libs/gi/upopt/src/expandSubstat.ts(1 hunks)libs/gi/upopt/src/makeObjective.ts(1 hunks)libs/gi/upopt/src/mathUtil.test.ts(1 hunks)libs/gi/upopt/src/mathUtil.ts(1 hunks)libs/gi/upopt/src/mvncdf.ts(1 hunks)libs/gi/upopt/src/substatProbs.ts(1 hunks)libs/gi/upopt/src/upOpt.test.ts(1 hunks)libs/gi/upopt/src/upOpt.ts(1 hunks)libs/gi/upopt/src/upOpt.types.ts(1 hunks)libs/gi/upopt/tsconfig.eslint.json(1 hunks)libs/gi/upopt/tsconfig.json(1 hunks)libs/gi/upopt/tsconfig.lib.json(1 hunks)libs/gi/upopt/tsconfig.spec.json(1 hunks)libs/gi/upopt/vite.config.ts(1 hunks)tsconfig.base.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
libs/gi/upopt/src/upOpt.test.ts (9)
libs/gi/upopt/src/upOpt.types.ts (4)
Objective(16-20)MarkovNode(41-41)GaussianNode(22-27)SubstatLevelNode(46-56)libs/gi/upopt/src/evaluation.ts (2)
evalMarkovNode(77-85)evaluateGaussian(16-59)libs/gi/upopt/src/makeObjective.ts (1)
makeObjective(13-48)libs/gi/upopt/src/expandSubstat.ts (3)
expandSubstatLevel(8-33)makeRollsNode(98-116)makeSubstatNode(125-175)libs/gi/upopt/src/expandRolls.ts (1)
expandRollsLevel(8-37)libs/gi/upopt/src/deduplicate.ts (1)
deduplicate(16-34)libs/gi/consts/src/artifact.ts (2)
SubstatKey(169-169)allSubstatKeys(156-167)libs/gi/upopt/src/consts.ts (1)
substatWeights(6-17)libs/gi/upopt/src/substatProbs.ts (1)
crawlSubstats(59-73)
libs/gi/upopt/src/mvncdf.ts (1)
libs/gi/upopt/src/mathUtil.ts (2)
erf(33-53)gaussPDF(56-64)
libs/gi/upopt/src/substatProbs.ts (2)
libs/gi/consts/src/artifact.ts (1)
SubstatKey(169-169)libs/gi/upopt/src/consts.ts (1)
substatWeights(6-17)
libs/gi/upopt/src/mathUtil.test.ts (1)
libs/gi/upopt/src/mathUtil.ts (6)
quadrinomial(23-27)factorial(68-71)gaussPDF(56-64)erf(33-53)crawlUpgrades(171-186)rollCountProb(87-121)
libs/gi/upopt/src/expandSubstat.ts (4)
libs/gi/upopt/src/upOpt.types.ts (2)
SubstatLevelNode(46-56)RollsLevelNode(61-69)libs/gi/upopt/src/mathUtil.ts (4)
crawlUpgrades(171-186)rollCountProb(87-121)diag(73-75)rollCountMuVar(124-168)libs/gi/consts/src/artifact.ts (2)
SubstatKey(169-169)ArtifactRarity(173-173)libs/sr/util/src/relic.ts (1)
getSubstatValue(61-75)
libs/gi/upopt/src/expandRolls.ts (4)
libs/gi/upopt/src/upOpt.types.ts (2)
RollsLevelNode(61-69)ValuesLevelNode(74-77)libs/common/util/src/lib/array.ts (2)
range(14-16)cartesian(27-31)libs/gi/upopt/src/mathUtil.ts (1)
quadrinomial(23-27)libs/sr/util/src/relic.ts (1)
getSubstatValue(61-75)
libs/gi/upopt/src/upOpt.ts (7)
libs/gi/db/src/Interfaces/ICachedArtifact.ts (1)
ICachedArtifact(2-7)libs/gi/upopt/src/expandSubstat.ts (1)
makeSubstatNode(125-175)libs/gi/consts/src/artifact.ts (8)
allSubstatKeys(156-167)ArtifactRarity(173-173)allArtifactSlotKeys(64-70)artSlotMainKeys(128-134)SubstatKey(169-169)ArtifactSlotKey(71-71)MainStatKey(168-168)artMaxLevel(73-79)libs/gi/upopt/src/substatProbs.ts (1)
crawlSubstats(59-73)libs/gi/upopt/src/consts.ts (1)
allMainStatProbs(60-66)libs/gi/good/src/IArtifact.ts (1)
IArtifact(11-16)libs/gi/upopt/src/upOpt.types.ts (1)
SubstatLevelNode(46-56)
libs/gi/upopt/src/evaluation.ts (2)
libs/gi/upopt/src/upOpt.types.ts (11)
Objective(16-20)GaussianNode(22-27)EvaluatedGaussian(29-39)SubstatLevelNode(46-56)EvaluatedSubstatNode(57-59)RollsLevelNode(61-69)EvaluatedRollsNode(70-72)ValuesLevelNode(74-77)EvaluatedValuesNode(78-80)MarkovNode(41-41)EvaluatedMarkovNode(42-45)libs/gi/upopt/src/mvncdf.ts (1)
mvnPE_bad(37-57)
libs/gi/upopt/src/makeObjective.ts (3)
libs/gi/wr/src/optimization.ts (2)
OptNode(18-22)precompute(72-152)libs/gi/upopt/src/upOpt.types.ts (1)
Objective(16-20)libs/gi/consts/src/artifact.ts (1)
allSubstatKeys(156-167)
libs/gi/upopt/src/deduplicate.ts (1)
libs/gi/upopt/src/upOpt.types.ts (5)
MarkovNode(41-41)Objective(16-20)SubstatLevelNode(46-56)RollsLevelNode(61-69)ValuesLevelNode(74-77)
libs/gi/upopt/src/upOpt.types.ts (1)
libs/gi/consts/src/artifact.ts (2)
SubstatKey(169-169)ArtifactRarity(173-173)
libs/gi/upopt/src/consts.ts (1)
libs/gi/consts/src/artifact.ts (1)
MainStatKey(168-168)
libs/gi/upopt/src/mathUtil.ts (1)
libs/common/util/src/lib/array.ts (1)
range(14-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
| { | ||
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "outDir": "../../dist/out-tsc", |
There was a problem hiding this comment.
Fix the test outDir path before it dumps artifacts in the wrong place
It's 3 a.m., the gacha banners reset in two hours, and this ../../dist/out-tsc is sending the compiled junk into libs/dist/out-tsc instead of the repo’s shared dist/out-tsc. Every other config in this library climbs three directories for a reason—please align this one or the build cache will never crit the way we need.
Apply this patch to point the outDir back to the workspace root:
- "outDir": "../../dist/out-tsc",
+ "outDir": "../../../dist/out-tsc",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "outDir": "../../dist/out-tsc", | |
| "outDir": "../../../dist/out-tsc", |
🤖 Prompt for AI Agents
In libs/gi/upopt/tsconfig.spec.json around line 4, the "outDir" is set to
"../../dist/out-tsc" which places test build artifacts under libs/dist/out-tsc;
update the path to climb one more directory so artifacts go to the repo root
shared dist (change to "../../../dist/out-tsc"). Edit the JSON value for
"outDir" accordingly and save to ensure test outputs and build cache target the
workspace-level dist/out-tsc.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/gi/upopt/src/deduplicate.ts(1 hunks)libs/gi/upopt/src/expandRolls.ts(1 hunks)libs/gi/upopt/src/upOpt.test.ts(1 hunks)libs/gi/upopt/src/upOpt.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/gi/upopt/src/expandRolls.ts
🧰 Additional context used
🧬 Code graph analysis (3)
libs/gi/upopt/src/upOpt.test.ts (10)
libs/gi/upopt/src/upOpt.types.ts (4)
Objective(16-20)MarkovNode(41-41)GaussianNode(22-27)SubstatLevelNode(46-56)libs/gi/upopt/src/evaluation.ts (2)
evalMarkovNode(77-85)evaluateGaussian(16-59)libs/gi/upopt/src/makeObjective.ts (1)
makeObjective(13-48)libs/gi/upopt/src/expandSubstat.ts (3)
expandSubstatLevel(8-33)makeRollsNode(98-116)makeSubstatNode(125-175)libs/gi/upopt/src/expandRolls.ts (1)
expandRollsLevel(8-40)libs/gi/upopt/src/deduplicate.ts (1)
deduplicate(16-34)libs/gi/consts/src/artifact.ts (2)
SubstatKey(169-169)allSubstatKeys(156-167)libs/gi/upopt/src/upOpt.ts (1)
expandNode(25-29)libs/gi/upopt/src/consts.ts (1)
substatWeights(6-17)libs/gi/upopt/src/substatProbs.ts (1)
crawlSubstats(59-73)
libs/gi/upopt/src/deduplicate.ts (1)
libs/gi/upopt/src/upOpt.types.ts (5)
MarkovNode(41-41)Objective(16-20)SubstatLevelNode(46-56)RollsLevelNode(61-69)ValuesLevelNode(74-77)
libs/gi/upopt/src/upOpt.ts (8)
libs/gi/upopt/src/upOpt.types.ts (2)
MarkovNode(41-41)SubstatLevelNode(46-56)libs/gi/upopt/src/expandSubstat.ts (2)
expandSubstatLevel(8-33)makeSubstatNode(125-175)libs/gi/upopt/src/expandRolls.ts (1)
expandRollsLevel(8-40)libs/gi/db/src/Interfaces/ICachedArtifact.ts (1)
ICachedArtifact(2-7)libs/gi/consts/src/artifact.ts (8)
allSubstatKeys(156-167)ArtifactRarity(173-173)allArtifactSlotKeys(64-70)artSlotMainKeys(128-134)SubstatKey(169-169)ArtifactSlotKey(71-71)MainStatKey(168-168)artMaxLevel(73-79)libs/gi/upopt/src/substatProbs.ts (1)
crawlSubstats(59-73)libs/gi/upopt/src/consts.ts (1)
allMainStatProbs(60-66)libs/gi/good/src/IArtifact.ts (1)
IArtifact(11-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: call-build / build
- GitHub Check: call-build / build
- GitHub Check: typecheck
- GitHub Check: gen-file
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (2)
libs/gi/upopt/src/upOpt.ts (2)
38-48: Re-add the leveled artifact’s set piece to the base stats.Running on three hours of sleep, I still see objectives mis-evaluating because the node we return never increments
base[art.setKey]. That drops the current artifact’s set membership, so any 2pc/4pc bonus math goes out the window—exactly what I flagged earlier.const base = toStats(currentBuild, art) const { subkeys, stats } = getSubkeys(art) Object.entries(stats).forEach(([k, v]) => (base[k] = (base[k] ?? 0) + v)) const rollsLeft = getRollsRemaining(art.level, art.rarity) const { rarity } = art + base[art.setKey] = (base[art.setKey] ?? 0) + 1
100-110: Fresh drops still forget their set tags.Even my gacha-addled brain remembers the earlier bug: we ignore
info.sets, so every generated node behaves like it came from no set at all. Set-bonus objectives will be catastrophically wrong. Please loop the offered sets, stamp the base with the set key, and split the probability.- crawlSubstats([], subsToConsider).forEach(({ p, subs }) => { - const nodeInfo = { - base, - rarity, - subkeys: subs.map((key) => ({ key, baseRolls: 1 })), - rollsLeft: getRollsRemaining(0, rarity), - } - const n = makeSubstatNode(nodeInfo) - out.push({ p: ((p * pMain) / 5) * (1 - p3sub), n }) + crawlSubstats([], subsToConsider).forEach(({ p, subs }) => { + info.sets.forEach((setKey) => { + const baseWithSet = { + ...base, + [setKey]: (base[setKey] ?? 0) + 1, + } + const nodeInfo = { + base: baseWithSet, + rarity, + subkeys: subs.map((key) => ({ key, baseRolls: 1 })), + rollsLeft: getRollsRemaining(0, rarity), + } + const n = makeSubstatNode(nodeInfo) + out.push({ + p: ((p * pMain) / 5) * (1 - p3sub) * (1 / info.sets.length), + n, + }) + }) })
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
libs/gi/upopt/src/deduplicate.ts (2)
24-34: Non-idiomatic descending sort; drop the negation for clarity
nodes.sort((a, b) => -cmpNodes(...))is harder to reason about and risks future comparator changes. Ascending works the same for adjacency-based merging.- nodes.sort((a, b) => -cmpNodes(a.n, b.n)) + nodes.sort((a, b) => cmpNodes(a.n, b.n))
20-23: Optional: clone input nodes indeduplicatebefore simplifying
Burned the midnight oil verifying call sites—none reuse the original objects, so you can safely prependnodes = nodes.map(({ p, n }) => ({ p, n: structuredClone(n) }))to isolate in-place side-effects.
libs/gi/upopt/src/upOpt.test.ts (2)
95-96: Zero-dim covariance: prefer [] over[[]]for clarityWith no substats,
covis unused, but[]communicates 0×0 better than a jagged[[]].- cov: [[]], + cov: [],Also applies to: 118-119
296-314: Good coverage on simplify-before-dedup pathThis exercises the prune-then-compare logic. Once reshape-affix pruning is fixed in the deduper, consider adding a case where a reshaped affix is pruned to avoid regressions.
Happy to add a test that prunes a reshaped affix and asserts non-zero expansion mass. Want me to draft it?
libs/gi/upopt/src/upOpt.ts (1)
85-119: Proposed patch for 3-sub start (if you want it now)Here’s a concrete approach assuming we expose
crawlSubstatsKfromsubstatProbs:-import { crawlSubstats } from './substatProbs' +import { crawlSubstats, crawlSubstatsK } from './substatProbs' @@ - crawlSubstats([], subsToConsider).forEach(({ p, subs }) => { + // 4-line starts + crawlSubstats([], subsToConsider).forEach(({ p, subs }) => { const nodeInfo = { base, rarity, subkeys: subs.map((key) => ({ key, baseRolls: 1 })), rollsLeft: getRollsRemaining(0, rarity), } const n = makeSubstatNode(nodeInfo) out.push({ p: p * pMain * pSet * pSlot * (1 - p3sub), n }) }) + // 3-line starts + crawlSubstatsK([], subsToConsider, 3).forEach(({ p, subs }) => { + const nodeInfo = { + base, + rarity, + subkeys: subs.map((key) => ({ key, baseRolls: 1 })), // 3 lines + rollsLeft: getRollsRemaining(0, rarity), // reserve one roll for the 4th on level-up + } + const n = makeSubstatNode(nodeInfo) + out.push({ p: p * pMain * pSet * pSlot * p3sub, n }) + })And add in
libs/gi/upopt/src/substatProbs.ts:export function crawlSubstatsK( prefix: readonly SubstatKey[], options: readonly SubstatKey[], k: number, insertPrefix = true ): { p: number; subs: SubstatKey[] }[] { const optW = options.map((k) => substatWeights[k]).sort((a, b) => a - b) return combinations(options, k - prefix.length) .map((combo) => { const comboW = combo.map((k) => substatWeights[k]).sort((a, b) => a - b) const p = substatProb(comboW, optW, k - prefix.length) return insertPrefix ? { p, subs: [...prefix, ...combo] } : { p, subs: combo } }) .filter(({ p }) => p > 0) }I can PR this helper + tests if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/gi/upopt/src/deduplicate.ts(1 hunks)libs/gi/upopt/src/upOpt.test.ts(1 hunks)libs/gi/upopt/src/upOpt.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
libs/gi/upopt/src/upOpt.test.ts (10)
libs/gi/upopt/src/upOpt.types.ts (4)
Objective(16-20)MarkovNode(41-41)GaussianNode(22-27)SubstatLevelNode(46-56)libs/gi/upopt/src/evaluation.ts (2)
evalMarkovNode(77-85)evaluateGaussian(16-59)libs/gi/upopt/src/makeObjective.ts (1)
makeObjective(13-48)libs/gi/upopt/src/expandSubstat.ts (3)
expandSubstatLevel(8-33)makeRollsNode(98-116)makeSubstatNode(125-175)libs/gi/upopt/src/expandRolls.ts (1)
expandRollsLevel(8-40)libs/gi/upopt/src/deduplicate.ts (1)
deduplicate(16-34)libs/gi/consts/src/artifact.ts (2)
SubstatKey(169-169)allSubstatKeys(156-167)libs/gi/upopt/src/upOpt.ts (1)
expandNode(25-29)libs/gi/upopt/src/consts.ts (1)
substatWeights(6-17)libs/gi/upopt/src/substatProbs.ts (1)
crawlSubstats(59-73)
libs/gi/upopt/src/deduplicate.ts (1)
libs/gi/upopt/src/upOpt.types.ts (5)
MarkovNode(41-41)Objective(16-20)SubstatLevelNode(46-56)RollsLevelNode(61-69)ValuesLevelNode(74-77)
libs/gi/upopt/src/upOpt.ts (8)
libs/gi/upopt/src/upOpt.types.ts (2)
MarkovNode(41-41)SubstatLevelNode(46-56)libs/gi/upopt/src/expandSubstat.ts (2)
expandSubstatLevel(8-33)makeSubstatNode(125-175)libs/gi/upopt/src/expandRolls.ts (1)
expandRollsLevel(8-40)libs/gi/db/src/Interfaces/ICachedArtifact.ts (1)
ICachedArtifact(2-7)libs/gi/consts/src/artifact.ts (8)
allSubstatKeys(156-167)ArtifactRarity(173-173)allArtifactSlotKeys(64-70)artSlotMainKeys(128-134)SubstatKey(169-169)ArtifactSlotKey(71-71)MainStatKey(168-168)artMaxLevel(73-79)libs/gi/upopt/src/substatProbs.ts (1)
crawlSubstats(59-73)libs/gi/upopt/src/consts.ts (1)
allMainStatProbs(60-66)libs/gi/good/src/IArtifact.ts (1)
IArtifact(11-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (5)
libs/gi/upopt/src/upOpt.test.ts (2)
225-256: Nice GMM vs Gaussian consistency checksThe linear-objective equivalence checks across expandRolls are tight. The additional post-dedup check is a good guard.
384-401: Deterministic probabilities with manual combos: solidPrecomputed fractions look gnarly but effective for catching drift in
crawlSubstats. Keep these.libs/gi/upopt/src/upOpt.ts (3)
194-239: toStats set accounting looks correct nowCounts existing build sets and adds
+1for the generated artifact; avoids the missing-set bug flagged earlier. Sleep-deprived me is relieved.
127-155: Reshape: enforceinitialValuepresence and slot set count — goodThrowing on undefined
initialValueprevents silent math drift. Base stats include set membership viatoStats; this fixes prior set-bonus omissions in rerolls.
31-79: Level-up flow: unactivated substat handling is sensibleSubtracting a roll per activation and folding
initialValueinto base is aligned with roll accounting. No issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
libs/gi/upopt/src/deduplicate.ts (1)
24-34: Comparator sign flip is confusing; prefer direct orderThe leading minus makes the sort harder to reason about and invites bugs when cmpNodes evolves. Sort ascending directly for clarity.
- nodes.sort((a, b) => -cmpNodes(a.n, b.n)) + nodes.sort((a, b) => cmpNodes(a.n, b.n))libs/gi/upopt/src/expandSubstat.ts (1)
15-31: Use forEach instead of map for side effectsTiny nit: map’s return is unused and this confused my half-asleep eyes. forEach reads cleaner.
- crawlUpgrades(rollsLeft).map((counts) => { + crawlUpgrades(rollsLeft).forEach((counts) => { ... - }) + })libs/gi/upopt/src/upOpt.ts (2)
85-119: Model 3‑line starts instead of dropping themfreshArtifact multiplies by (1 − p3sub) and only emits 4‑line starts. We should branch a 3‑line path that adds the 4th stat at first upgrade (consuming 1 roll) with proper probabilities, then feed into makeSubstatNode.
I can wire a small helper that mirrors levelUpArtifact’s “sum over possible 4th stat” and integrate it here.
64-79: Complete the “sum over possible 4th stat” pathlevelUpArtifact’s TODO is exactly what freshArtifact needs too. Merging these into a reusable helper reduces cognitive load (and saves me a pity pull’s worth of braincells).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/gi/upopt/src/deduplicate.ts(1 hunks)libs/gi/upopt/src/expandRolls.ts(1 hunks)libs/gi/upopt/src/expandSubstat.ts(1 hunks)libs/gi/upopt/src/makeObjective.ts(1 hunks)libs/gi/upopt/src/upOpt.test.ts(1 hunks)libs/gi/upopt/src/upOpt.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
libs/gi/upopt/src/upOpt.test.ts (10)
libs/gi/upopt/src/upOpt.types.ts (4)
Objective(16-20)MarkovNode(41-41)GaussianNode(22-27)SubstatLevelNode(46-56)libs/gi/upopt/src/evaluation.ts (2)
evalMarkovNode(77-85)evaluateGaussian(16-59)libs/gi/upopt/src/makeObjective.ts (1)
makeObjective(13-48)libs/gi/upopt/src/expandSubstat.ts (3)
expandSubstatLevel(8-33)makeRollsNode(98-116)makeSubstatNode(125-175)libs/gi/upopt/src/expandRolls.ts (1)
expandRollsLevel(8-40)libs/gi/upopt/src/deduplicate.ts (1)
deduplicate(16-34)libs/gi/consts/src/artifact.ts (2)
SubstatKey(169-169)allSubstatKeys(156-167)libs/gi/upopt/src/upOpt.ts (1)
expandNode(25-29)libs/gi/upopt/src/consts.ts (1)
substatWeights(6-17)libs/gi/upopt/src/substatProbs.ts (1)
crawlSubstats(59-73)
libs/gi/upopt/src/deduplicate.ts (1)
libs/gi/upopt/src/upOpt.types.ts (5)
MarkovNode(41-41)Objective(16-20)SubstatLevelNode(46-56)RollsLevelNode(61-69)ValuesLevelNode(74-77)
libs/gi/upopt/src/expandSubstat.ts (4)
libs/gi/upopt/src/upOpt.types.ts (2)
SubstatLevelNode(46-56)RollsLevelNode(61-69)libs/gi/upopt/src/mathUtil.ts (4)
crawlUpgrades(170-185)rollCountProb(87-120)diag(73-75)rollCountMuVar(123-167)libs/gi/consts/src/artifact.ts (2)
SubstatKey(169-169)ArtifactRarity(173-173)libs/sr/util/src/relic.ts (1)
getSubstatValue(61-75)
libs/gi/upopt/src/expandRolls.ts (4)
libs/gi/upopt/src/upOpt.types.ts (2)
RollsLevelNode(61-69)ValuesLevelNode(74-77)libs/common/util/src/lib/array.ts (2)
range(14-16)cartesian(27-31)libs/gi/upopt/src/mathUtil.ts (1)
quadrinomial(23-27)libs/sr/util/src/relic.ts (1)
getSubstatValue(61-75)
libs/gi/upopt/src/upOpt.ts (8)
libs/gi/upopt/src/upOpt.types.ts (2)
MarkovNode(41-41)SubstatLevelNode(46-56)libs/gi/upopt/src/expandSubstat.ts (2)
expandSubstatLevel(8-33)makeSubstatNode(125-175)libs/gi/upopt/src/expandRolls.ts (1)
expandRollsLevel(8-40)libs/gi/db/src/Interfaces/ICachedArtifact.ts (1)
ICachedArtifact(2-7)libs/gi/consts/src/artifact.ts (8)
allSubstatKeys(156-167)ArtifactRarity(173-173)allArtifactSlotKeys(64-70)artSlotMainKeys(128-134)SubstatKey(169-169)ArtifactSlotKey(71-71)MainStatKey(168-168)artMaxLevel(73-79)libs/gi/upopt/src/substatProbs.ts (1)
crawlSubstats(59-73)libs/gi/upopt/src/consts.ts (1)
allMainStatProbs(60-66)libs/gi/good/src/IArtifact.ts (1)
IArtifact(11-16)
libs/gi/upopt/src/makeObjective.ts (3)
libs/gi/wr/src/optimization.ts (2)
OptNode(18-22)precompute(72-152)libs/gi/upopt/src/upOpt.types.ts (1)
Objective(16-20)libs/gi/consts/src/artifact.ts (1)
allSubstatKeys(156-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (4)
libs/gi/upopt/src/deduplicate.ts (2)
85-87: Values-level equality on base only — confirm intentcmpValuesNode ignores mu/cov. If two different roll paths yield identical base stats but different Gaussian params, they’ll merge. If that’s intentional (values nodes as deterministic leaves), all good; otherwise include mu/cov/subs in the compare.
36-45: Zero-derivative pruning looks fixed and alignedThe inverted mask is correct now; subs/mu/cov stay aligned after row/col filtering. My sleep-deprived gacha brain approves.
libs/gi/upopt/src/expandSubstat.ts (1)
35-42: Confirm substat tiering assumptionsUsing [0.7, 0.8, 0.9, 1] uniformly times 'max' per roll is a strong assumption. If GI changes tier weights per rarity, mu/var skew. Please confirm these are uniform for 3★/4★/5★ in our data tables.
libs/gi/upopt/src/upOpt.ts (1)
224-239: Probabilities sanity-checkBetween pSlot, pSet, pMain and crawlSubstats p, total mass should be ≤1 (exactly 1 if the universe is complete). Worth a quick check in tests to avoid silent drift.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
libs/gi/upopt/src/deduplicate.ts (3)
16-34: Be explicit about mutation or return fresh nodes.
deduplicatesimplifies in place, which can surprise callers holding references. Either document the mutation or defensively clone nodes before simplification.
24-24: Drop the negation for clarity.
nodes.sort(cmpNodes)is easier to read than sorting by-cmpNodesand then re‑callingcmpNodesfor equality. If you want reverse order, flip type ordering insidecmpNodes.
15-15: Consider exporting theweightedNodetype.Public API exposes it via the function signature; exporting improves DX and avoids anonymous types in d.ts.
libs/gi/upopt/src/makeObjective.ts (1)
38-41: Slightly clearer split of f/df without mutatingvaluesNit: using indices avoids splice/shift churn in hot paths.
- const f = values.splice(0, nodes.length) - const df = nonzeroDerivs.map((subs) => - Object.fromEntries(subs.map((s) => [s, values.shift()!])) - ) as DynStat[] + const f = values.slice(0, nodes.length) + let k = nodes.length + const df = nonzeroDerivs.map((subs) => + Object.fromEntries(subs.map((s) => [s, values[k++]])) + ) as DynStat[]libs/gi/upopt/src/expandSubstat.ts (2)
15-31: UseforEach(or a reducer) instead ofmapfor side effectsTiny nit, but
.mapallocates an array you throw away on every branch crawl.- crawlUpgrades(rollsLeft).map((counts) => { + crawlUpgrades(rollsLeft).forEach((counts) => { ... - }) + })
126-135: Avoid mutating inputs (info.subkeys,reshape.affixes) in constructorSorting the arrays on
infomutates caller-owned data and can surprise upstream code ifinfois reused.-export function makeSubstatNode(info: SubstatLevelInfo): SubstatLevelNode { - const { rollsLeft, subkeys, reshape } = info - subkeys.sort((a, b) => a.key.localeCompare(b.key)) // Ensure consistent ordering - reshape?.affixes.sort((a, b) => a.localeCompare(b)) // Ensure consistent ordering +export function makeSubstatNode(info: SubstatLevelInfo): SubstatLevelNode { + const { rollsLeft } = info + const subkeys = [...info.subkeys].sort((a, b) => a.key.localeCompare(b.key)) + const reshape = info.reshape + ? { ...info.reshape, affixes: [...info.reshape.affixes].sort((a, b) => a.localeCompare(b)) } + : undefinedMy brain’s already juggling pity counters; let’s not juggle object identity too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/gi/upopt/src/deduplicate.ts(1 hunks)libs/gi/upopt/src/expandSubstat.ts(1 hunks)libs/gi/upopt/src/makeObjective.ts(1 hunks)libs/gi/upopt/src/upOpt.test.ts(1 hunks)libs/gi/upopt/src/upOpt.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/gi/upopt/src/upOpt.test.ts
- libs/gi/upopt/src/upOpt.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-27T18:05:10.497Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:36-45
Timestamp: 2025-09-27T18:05:10.497Z
Learning: In gi/upopt's expandSubstatLevel, the getReshapeIxs function handles missing reshape affixes by assigning them virtual indices beyond subkeys.length when there's room (< 4 substats). This allows the reshape constraint calculation to work correctly even after deduplication removes zero-derivative substats, as missing affixes are treated as "omitted substats" with 0 rolls.
Applied to files:
libs/gi/upopt/src/expandSubstat.tslibs/gi/upopt/src/deduplicate.ts
📚 Learning: 2025-09-27T18:05:10.497Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:36-45
Timestamp: 2025-09-27T18:05:10.497Z
Learning: In the gi/upopt library, expandSubstatLevel's getReshapeIxs function has built-in logic to handle missing substats from reshape.affixes, treating omitted substats as pruned affixes. The deduplication simplification of nodes doesn't break reshape constraints because the expansion logic is designed to work with missing reshape affixes.
Applied to files:
libs/gi/upopt/src/expandSubstat.tslibs/gi/upopt/src/deduplicate.ts
🧬 Code graph analysis (3)
libs/gi/upopt/src/expandSubstat.ts (3)
libs/gi/upopt/src/upOpt.types.ts (2)
SubstatLevelNode(46-56)RollsLevelNode(61-69)libs/gi/upopt/src/mathUtil.ts (4)
crawlUpgrades(170-185)rollCountProb(87-120)diag(73-75)rollCountMuVar(123-167)libs/sr/util/src/relic.ts (1)
getSubstatValue(61-75)
libs/gi/upopt/src/deduplicate.ts (1)
libs/gi/upopt/src/upOpt.types.ts (5)
MarkovNode(41-41)Objective(16-20)SubstatLevelNode(46-56)RollsLevelNode(61-69)ValuesLevelNode(74-77)
libs/gi/upopt/src/makeObjective.ts (3)
libs/gi/wr/src/optimization.ts (2)
OptNode(18-22)precompute(72-152)libs/gi/upopt/src/upOpt.types.ts (1)
Objective(16-20)libs/gi/consts/src/artifact.ts (1)
allSubstatKeys(156-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: call-build / build
- GitHub Check: typecheck
- GitHub Check: lint
- GitHub Check: gen-file
- GitHub Check: test
🔇 Additional comments (9)
libs/gi/upopt/src/deduplicate.ts (5)
36-45: Zero‑derivative pruning looks correct; reshape omission is fine.Masks stay aligned across subkeys/mu/cov and column pruning is done after row pruning. Not mirroring
reshapehere is OK since expansion handles missing affixes viagetReshapeIxs.Based on learnings
47-58: Rolls simplification is sane and aligned.Pruning zero‑deriv and zero‑roll subs while keeping
subDistrarrays/cov in sync looks good.
74-83: Base comparator fix LGTM.Key‑set check prevents
NaNdiffs; deterministic order preserved.
89-113: Substat comparator covers missing ties now.Including
rarity,rollsLeft, and robust reshape checks closes prior gaps.
115-124: Rolls comparator includes rarity — good catch.Prevents cross‑rarity coalescing.
libs/gi/upopt/src/makeObjective.ts (1)
17-24: LGTM: sparse Jacobian build respects zero-deriv pruningNice. Building
ddxonly for nonzero substats keeps eval fast and memory light—good for my sleep debt and your perf budget.libs/gi/upopt/src/expandSubstat.ts (3)
149-165: Covariance construction looks correct (counts correlation + per‑roll variance)Good decomposition: Cov = M_i M_j Cov(N_i, N_j) + 1{i=j} Var_roll * E[N_i]. This matches the model.
79-87: Swap-tracker update is incorrect for j>1; produces wrong swaps downstream
trackermaps sourceIndex → currentPos, but the code swaps by positions (i,j). This corrupts the mapping once we move past the first index, causing wrongmu/covreordering under reshape.Apply this fix to maintain the mapping correctly:
function ixsToSwaps(ixs: number[], nmax: number): number[][] { const tracker = Array.from({ length: nmax }, (_, i) => i) const out: number[][] = [] ixs.forEach((i, j) => { - if (i === tracker[j]) return - out.push([i, tracker[j]]) - swap(tracker, i, j) + const curPos = tracker[j] + if (i === curPos) return + out.push([i, curPos]) + // Maintain sourceIndex -> position mapping + const srcAtI = tracker.findIndex((pos) => pos === i) + tracker[j] = i + tracker[srcAtI] = curPos }) return out }Based on learnings.
35-42: VerifygetSubstatValuesignature matches arguments
Burning the midnight oil here—make suregetSubstatValuein libs/gi/util/src/artifact/artifact.ts actually accepts(substatKey, rarity, tier, roundUp)so that using'max', falsereturns the intended max roll, not a truncated value.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/gi/upopt/src/deduplicate.ts(1 hunks)libs/gi/upopt/src/expandSubstat.ts(1 hunks)libs/gi/upopt/src/makeObjective.ts(1 hunks)libs/gi/upopt/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/gi/upopt/tsconfig.json
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-27T18:05:10.497Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:36-45
Timestamp: 2025-09-27T18:05:10.497Z
Learning: In the gi/upopt library, expandSubstatLevel's getReshapeIxs function has built-in logic to handle missing substats from reshape.affixes, treating omitted substats as pruned affixes. The deduplication simplification of nodes doesn't break reshape constraints because the expansion logic is designed to work with missing reshape affixes.
Applied to files:
libs/gi/upopt/src/deduplicate.tslibs/gi/upopt/src/expandSubstat.ts
📚 Learning: 2025-09-27T18:05:10.497Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:36-45
Timestamp: 2025-09-27T18:05:10.497Z
Learning: In gi/upopt's expandSubstatLevel, the getReshapeIxs function handles missing reshape affixes by assigning them virtual indices beyond subkeys.length when there's room (< 4 substats). This allows the reshape constraint calculation to work correctly even after deduplication removes zero-derivative substats, as missing affixes are treated as "omitted substats" with 0 rolls.
Applied to files:
libs/gi/upopt/src/deduplicate.tslibs/gi/upopt/src/expandSubstat.ts
📚 Learning: 2025-09-27T19:22:20.632Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:85-87
Timestamp: 2025-09-27T19:22:20.632Z
Learning: In the gi/upopt library, ValuesLevelNode instances always have empty mu (mean) and cov (covariance) arrays in their subDistr. Only the base DynStat contains actual values, making base-only comparison in cmpValuesNode sufficient for deduplication.
Applied to files:
libs/gi/upopt/src/deduplicate.ts
📚 Learning: 2025-09-27T19:22:20.632Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:85-87
Timestamp: 2025-09-27T19:22:20.632Z
Learning: In the gi/upopt library, ValuesLevelNode instances always have empty mu (mean) and cov (covariance) arrays in their subDistr. Only the base DynStat contains actual values, making base-only comparison in cmpValuesNode sufficient for deduplication. Values nodes represent deterministic leaf nodes where all probabilistic uncertainty has been resolved.
Applied to files:
libs/gi/upopt/src/deduplicate.ts
📚 Learning: 2025-09-27T19:19:31.838Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/makeObjective.ts:25-31
Timestamp: 2025-09-27T19:19:31.838Z
Learning: The `optimize` function from genshin-optimizer/gi/wr preserves the order of input nodes and does not reorder them, even though it internally calls `deduplicate`. This means that index-based alignment between original nodes/jacobian entries and the optimized `allNodes` array is safe to rely on.
Applied to files:
libs/gi/upopt/src/makeObjective.ts
🧬 Code graph analysis (3)
libs/gi/upopt/src/deduplicate.ts (1)
libs/gi/upopt/src/upOpt.types.ts (5)
MarkovNode(41-41)Objective(16-20)SubstatLevelNode(46-56)RollsLevelNode(61-69)ValuesLevelNode(74-77)
libs/gi/upopt/src/makeObjective.ts (3)
libs/gi/wr/src/optimization.ts (2)
OptNode(18-22)precompute(72-152)libs/gi/upopt/src/upOpt.types.ts (1)
Objective(16-20)libs/gi/consts/src/artifact.ts (1)
allSubstatKeys(156-167)
libs/gi/upopt/src/expandSubstat.ts (4)
libs/gi/upopt/src/upOpt.types.ts (2)
SubstatLevelNode(46-56)RollsLevelNode(61-69)libs/gi/upopt/src/mathUtil.ts (4)
crawlUpgrades(177-192)rollCountProb(94-127)diag(80-82)rollCountMuVar(130-174)libs/gi/consts/src/artifact.ts (2)
SubstatKey(169-169)ArtifactRarity(173-173)libs/sr/util/src/relic.ts (1)
getSubstatValue(61-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call-build / build
- GitHub Check: typecheck
- GitHub Check: lint
🔇 Additional comments (12)
libs/gi/upopt/src/makeObjective.ts (2)
34-43: LGTM on the derivative construction!takes another sip of energy drink while checking pity counter
The slicing and indexing logic for
fanddflooks solid. You're correctly startingkatnodes.lengthand incrementing for each nonzero derivative, which aligns with how the Jacobian was flattened in line 26. The sparseDynStat[]construction usingObject.fromEntriesis clean and matches thenonzeroDerivsstructure.Based on learnings,
optimizepreserves order, so this index-based approach is safe.
25-29: Exclude predicate correctly filters dynamic nodes
Dynamic nodes (path[0]=='dyn') are excluded by the predicate, while static nodes (empty path → p=undefined) are retained. No changes needed.libs/gi/upopt/src/deduplicate.ts (4)
28-28: Clarify the negation in sort comparator.rubs tired eyes after another all-nighter farming artifacts
Line 28 sorts with
-cmpNodes(a.n, b.n). This negates the comparator, effectively reversing the order. Just want to confirm this is intentional. IfcmpNodesalready defines the desired order, the negation would flip it. Make sure this aligns with your merging logic expectations.
78-87: LGTM on cmpBase—no more NaN leakage!crashes into chair after another gacha session that drained all my primogems
The updated
cmpBaseproperly compares sorted key arrays first, then does numeric subtraction only when keys are identical. This prevents the NaN-from-undefined bug that was lurking in the original version. Solid fix!
93-117: LGTM on the comprehensive SubstatNode comparator!barely keeping eyes open while min-maxing artifact substats at 4 AM
The comparator now covers all the critical fields:
rarity,rollsLeft,reshape(with bothmintotalandaffixes.length),subkeys, andbase. This should prevent the false-positive merges that the earlier version would've caused. Nice total ordering!
119-129: LGTM on RollsNode comparator with rarity!zones out momentarily thinking about that 5-star artifact that rolled all into DEF
The
rarityfield is now properly compared at line 120, preventing the coalescing of 4★ and 5★ distributions. The rest of the comparator looks solid with length checks and element-wise comparisons. Good stuff!libs/gi/upopt/src/expandSubstat.ts (6)
8-33: LGTM on expandSubstatLevel!slumps over keyboard after yet another resin refresh wasted on bad artifacts
The expansion logic looks solid. You're computing
reshapeIxsonce upfront, then for each roll distribution fromcrawlUpgrades, you build the roll counts, compute the probability with reshape constraints factored in viareshapeIxs.reduce((a, i) => a + counts[i], 0), and filter out impossible configurations. The structure is clean and the early return forp === 0saves unnecessary node construction.Based on learnings, the reshape handling with
getReshapeIxsis designed to work correctly even with missing affixes.
44-54: Variance formula assumes uniform distribution—confirm this matches your model.rubs eyes after staring at probability distributions for 6 hours straight
Line 51 computes variance as
Σ(b - μ)² / N, which is correct for a uniform distribution over discrete values. Just want to confirm that your artifact roll model indeed treats the 4 possible values[0.7, 0.8, 0.9, 1.0]as equally likely (25% each). If the game has different probabilities for low vs high rolls, this variance would be off.
56-73: LGTM on getReshapeIxs—handles missing affixes elegantly!finally understanding the reshape logic after 3 cups of coffee
This function correctly handles the case where a reshape affix isn't present in
subkeysby assigning virtual indices beyond the current length (lines 65-67). Based on learnings, this design allows the reshape constraint calculation to work correctly even after deduplication removes zero-derivative substats. The error case at line 69 is a proper safeguard for truly impossible states (all 4 slots filled with none matching).
79-96: LGTM on ixsToSwaps—confirmed correct per past discussion!finally getting some rest after that exhausting debugging session in the past review
The swap-tracking logic here is solid. After the extensive discussion in past reviews, it's confirmed that
tracker[j]correctly tracks where the originalarr[j]has moved to, andswap(tracker, i, j)properly updates the mapping after each swap. No issues here!
98-117: Verify independence assumption in diagonal covariance.zones out momentarily thinking about correlated artifact rolls
Line 112-114 constructs a diagonal covariance matrix using
diag(), which assumes independence between different substats. This means you're modeling each substat's roll outcomes as independent random variables. Just confirm this aligns with your probabilistic model—if there's any correlation structure between substats (e.g., constraint on total stat points), a diagonal cov would miss that.
35-42: Exhaustive rarity coverage confirmed
ArtifactRarity only includes 3, 4, and 5 stars, so the explicit check covers all possible values and the error throw is dead code.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
libs/gi/upopt/src/markov-tree/markov.types.ts (2)
4-9: Make GaussianNode arrays readonly + document length invariants.Right now mu/cov/subs are mutable. Lock them down for safety and clarify invariants (mu.length === subs.length, cov is square of that size). Low-effort guardrails that save late-night bug hunts.
-export type GaussianNode = { - base: DynStat - subs: SubstatKey[] - mu: number[] // Mean stats vector - cov: number[][] // Covariance of stats matrix -} +export type GaussianNode = { + base: DynStat + subs: readonly SubstatKey[] + mu: readonly number[] // length === subs.length + cov: readonly (readonly number[])[] // square matrix, size === subs.length +}
23-27: Narrow derivative typing to reflect sparsity.computeWithDerivs returns per-node sparse derivatives; advertise that in the type to prevent accidental assumptions of full DynStat.
-export type Objective = { - computeWithDerivs: (x: DynStat) => [number[], DynStat[]] +export type Objective = { + computeWithDerivs: ( + x: DynStat + ) => [readonly number[], ReadonlyArray<Partial<DynStat>>] threshold: number[] zeroDeriv: SubstatKey[] // Substats that have zero derivative for all objective functions. }libs/gi/upopt/src/upOpt.test.ts (1)
434-439: Convert empty tests totest.todoto avoid false greens.These no-op tests pass without assertions and inflate confidence. Mark as todo or remove.
-describe('upOpt makeSubstatNode(s)', () => { - test('levelArtifact', () => {}) - test('fresh/domain/strongbox', () => {}) - test('reshape/dust', () => {}) - test('define/elixir', () => {}) -}) +describe('upOpt makeSubstatNode(s)', () => { + test.todo('levelArtifact') + test.todo('fresh/domain/strongbox') + test.todo('reshape/dust') + test.todo('define/elixir') +})libs/gi/upopt/src/markov-tree/makeObjective.ts (1)
25-31: Document the order-preservation invariant foroptimize.computeWithDerivs relies on
allNodeskeeping [nodes, jac] order. Add a short comment referencing the contract so sleepy future devs don’t “optimize” this away. Based on learnings.- const allNodes = optimize( + // IMPORTANT: optimize preserves input order; f/df slicing below depends on this. + const allNodes = optimize( [...nodes, ...jac.flat()],libs/gi/upopt/src/deduplicate.ts (2)
35-44: Add a quick shape assert after pruning to avoid skewed cov.If a mismatch ever slips in, covariance rows/cols can desync. A tiny runtime guard here prevents silent comparator chaos later.
node.subDistr.cov = node.subDistr.cov.map((row) => row.filter((_, i) => !toRemove[i]) ) + // sanity: cov must remain square and match mu/subs length + if ( + node.subDistr.mu.length !== node.subDistr.subs.length || + node.subDistr.cov.length !== node.subDistr.subs.length || + node.subDistr.cov.some((r) => r.length !== node.subDistr.subs.length) + ) { + throw new Error('simplifySubstatNode: covariance shape mismatch') + }
59-71: Deterministic total order — looks solid; tiny nit: avoid negating comparator in sort.Style nit from a gacha-addled brain: prefer defining cmpNodes for ascending and call
nodes.sort(cmp)to reduce mental flips. No behavior change required.- nodes.sort((a, b) => -cmpNodes(a.n, b.n)) + nodes.sort((a, b) => cmpNodes(a.n, b.n)) let prev: weightedNode | undefined = undefined(If you keep the negation, maybe drop a comment that the order is intentionally descending.)
Also applies to: 88-124
libs/gi/upopt/src/markov-tree/evaluation.ts (2)
54-56: Clamp variance before sqrt to avoid NaN bounds at ~zero variance.f_cov[0][0] can be slightly negative from FP noise. Clamp before sqrt.
- lower: upAvg - 4 * Math.sqrt(f_cov[0][0]), - upper: upAvg + 4 * Math.sqrt(f_cov[0][0]), + lower: upAvg - 4 * Math.sqrt(Math.max(f_cov[0]?.[0] ?? 0, 0)), + upper: upAvg + 4 * Math.sqrt(Math.max(f_cov[0]?.[0] ?? 0, 0)),
23-45: Optional: assert objective dimensionality matches thresholds.Quick guard helps catch miswired Objectives early (length mismatch).
const [f_mu, df] = obj.computeWithDerivs(stats) + if (obj.threshold.length !== f_mu.length) + throw new Error(`Objective threshold length ${obj.threshold.length} != f_mu length ${f_mu.length}`)libs/gi/upopt/src/expandRolls.ts (1)
42-49: Make covariance truly 0×0 for values nodes.cov: [[]] is a ragged 1×0; prefer empty 0×0 to reflect no random substats.
const subDistr = { base, subs: [], mu: [], - cov: [[]], + cov: [], }libs/gi/upopt/src/substatProbs.ts (2)
28-57: Caching via JSON is fine; Map can be cleaner and faster.Low‑stakes, but Map with tuple keys avoids stringify costs and GC churn in hot paths.
-const substatProbCache: Record<string, number> = {} +const substatProbCache = new Map<string, number>() ... - if (substatProbCache[key] !== undefined) return substatProbCache[key] + if (substatProbCache.has(key)) return substatProbCache.get(key)! ... - substatProbCache[key] = p + substatProbCache.set(key, p)
64-73: Doc nit: probability depends only on weight multiset.Multiple combos with identical weight patterns will share the same p; that's expected. Consider noting this for consumers to avoid surprise when identical p’s repeat across keys.
libs/gi/upopt/src/upOpt.types.ts (1)
17-23: Reshape type mismatch with mathUtil.Here: { affixes: SubstatKey[]; mintotal: number }. In mathUtil.rollCountProb/rollCountMuVar the shape is { n, min, total }. Please normalize or add a shared type + adapter to prevent drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
libs/gi/upopt/src/deduplicate.ts(1 hunks)libs/gi/upopt/src/expandRolls.ts(1 hunks)libs/gi/upopt/src/expandSubstat.ts(1 hunks)libs/gi/upopt/src/markov-tree/evaluation.ts(1 hunks)libs/gi/upopt/src/markov-tree/makeObjective.ts(1 hunks)libs/gi/upopt/src/markov-tree/markov.types.ts(1 hunks)libs/gi/upopt/src/markov-tree/mathUtil.test.ts(1 hunks)libs/gi/upopt/src/markov-tree/mathUtil.ts(1 hunks)libs/gi/upopt/src/markov-tree/mvncdf.ts(1 hunks)libs/gi/upopt/src/substatProbs.ts(1 hunks)libs/gi/upopt/src/upOpt.test.ts(1 hunks)libs/gi/upopt/src/upOpt.types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/gi/upopt/src/markov-tree/mathUtil.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/gi/upopt/src/expandSubstat.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-27T17:26:02.090Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/mathUtil.ts:106-118
Timestamp: 2025-09-27T17:26:02.090Z
Learning: The range(from, to) function in genshin-optimizer/common/util is inclusive on both endpoints, meaning range(0, reshape.total) includes reshape.total as the final value. This differs from typical JavaScript range implementations which are usually half-open.
Applied to files:
libs/gi/upopt/src/markov-tree/mathUtil.ts
📚 Learning: 2025-09-27T18:05:10.516Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:36-45
Timestamp: 2025-09-27T18:05:10.516Z
Learning: In the gi/upopt library, expandSubstatLevel's getReshapeIxs function has built-in logic to handle missing substats from reshape.affixes, treating omitted substats as pruned affixes. The deduplication simplification of nodes doesn't break reshape constraints because the expansion logic is designed to work with missing reshape affixes.
Applied to files:
libs/gi/upopt/src/deduplicate.ts
📚 Learning: 2025-09-27T18:05:10.516Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:36-45
Timestamp: 2025-09-27T18:05:10.516Z
Learning: In gi/upopt's expandSubstatLevel, the getReshapeIxs function handles missing reshape affixes by assigning them virtual indices beyond subkeys.length when there's room (< 4 substats). This allows the reshape constraint calculation to work correctly even after deduplication removes zero-derivative substats, as missing affixes are treated as "omitted substats" with 0 rolls.
Applied to files:
libs/gi/upopt/src/deduplicate.ts
📚 Learning: 2025-09-27T19:22:20.666Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:85-87
Timestamp: 2025-09-27T19:22:20.666Z
Learning: In the gi/upopt library, ValuesLevelNode instances always have empty mu (mean) and cov (covariance) arrays in their subDistr. Only the base DynStat contains actual values, making base-only comparison in cmpValuesNode sufficient for deduplication.
Applied to files:
libs/gi/upopt/src/deduplicate.ts
📚 Learning: 2025-09-27T19:22:20.666Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/deduplicate.ts:85-87
Timestamp: 2025-09-27T19:22:20.666Z
Learning: In the gi/upopt library, ValuesLevelNode instances always have empty mu (mean) and cov (covariance) arrays in their subDistr. Only the base DynStat contains actual values, making base-only comparison in cmpValuesNode sufficient for deduplication. Values nodes represent deterministic leaf nodes where all probabilistic uncertainty has been resolved.
Applied to files:
libs/gi/upopt/src/deduplicate.ts
📚 Learning: 2025-09-27T19:19:31.846Z
Learnt from: tooflesswulf
PR: frzyc/genshin-optimizer#3069
File: libs/gi/upopt/src/makeObjective.ts:25-31
Timestamp: 2025-09-27T19:19:31.846Z
Learning: The `optimize` function from genshin-optimizer/gi/wr preserves the order of input nodes and does not reorder them, even though it internally calls `deduplicate`. This means that index-based alignment between original nodes/jacobian entries and the optimized `allNodes` array is safe to rely on.
Applied to files:
libs/gi/upopt/src/markov-tree/makeObjective.ts
🧬 Code graph analysis (10)
libs/gi/upopt/src/markov-tree/mvncdf.ts (1)
libs/gi/upopt/src/markov-tree/mathUtil.ts (2)
erf(40-60)gaussPDF(63-71)
libs/gi/upopt/src/substatProbs.ts (2)
libs/gi/consts/src/artifact.ts (1)
SubstatKey(169-169)libs/gi/upopt/src/consts.ts (1)
substatWeights(6-17)
libs/gi/upopt/src/markov-tree/markov.types.ts (1)
libs/gi/consts/src/artifact.ts (1)
SubstatKey(169-169)
libs/gi/upopt/src/markov-tree/mathUtil.ts (1)
libs/common/util/src/lib/array.ts (1)
range(14-16)
libs/gi/upopt/src/upOpt.test.ts (10)
libs/gi/upopt/src/markov-tree/markov.types.ts (2)
Objective(23-27)GaussianNode(4-9)libs/gi/upopt/src/upOpt.types.ts (2)
MarkovNode(8-8)SubstatLevelNode(13-23)libs/gi/upopt/src/markov-tree/evaluation.ts (2)
evalMarkovNode(75-83)evaluateGaussian(14-57)libs/gi/upopt/src/markov-tree/makeObjective.ts (1)
makeObjective(13-49)libs/gi/upopt/src/expandSubstat.ts (3)
expandSubstatLevel(13-38)makeRollsNode(103-122)makeSubstatNode(131-181)libs/gi/upopt/src/expandRolls.ts (1)
expandRollsLevel(8-40)libs/gi/upopt/src/deduplicate.ts (1)
deduplicate(15-33)libs/gi/upopt/src/upOpt.ts (1)
expandNode(25-29)libs/gi/upopt/src/consts.ts (1)
substatWeights(6-17)libs/gi/upopt/src/substatProbs.ts (1)
crawlSubstats(59-73)
libs/gi/upopt/src/upOpt.types.ts (2)
libs/gi/upopt/src/markov-tree/markov.types.ts (2)
GaussianNode(4-9)EvaluatedGaussian(11-21)libs/gi/consts/src/artifact.ts (2)
ArtifactRarity(173-173)SubstatKey(169-169)
libs/gi/upopt/src/expandRolls.ts (4)
libs/gi/upopt/src/upOpt.types.ts (2)
RollsLevelNode(28-36)ValuesLevelNode(41-44)libs/common/util/src/lib/array.ts (2)
range(14-16)cartesian(27-31)libs/gi/upopt/src/markov-tree/mathUtil.ts (1)
quadrinomial(19-34)libs/sr/util/src/relic.ts (1)
getSubstatValue(61-75)
libs/gi/upopt/src/deduplicate.ts (2)
libs/gi/upopt/src/upOpt.types.ts (4)
MarkovNode(8-8)SubstatLevelNode(13-23)RollsLevelNode(28-36)ValuesLevelNode(41-44)libs/gi/upopt/src/markov-tree/markov.types.ts (1)
Objective(23-27)
libs/gi/upopt/src/markov-tree/evaluation.ts (3)
libs/gi/upopt/src/markov-tree/markov.types.ts (3)
Objective(23-27)GaussianNode(4-9)EvaluatedGaussian(11-21)libs/gi/upopt/src/markov-tree/mvncdf.ts (1)
mvnPE_bad(37-57)libs/gi/upopt/src/upOpt.types.ts (8)
SubstatLevelNode(13-23)EvaluatedSubstatNode(24-26)RollsLevelNode(28-36)EvaluatedRollsNode(37-39)ValuesLevelNode(41-44)EvaluatedValuesNode(45-47)MarkovNode(8-8)EvaluatedMarkovNode(9-12)
libs/gi/upopt/src/markov-tree/makeObjective.ts (3)
libs/gi/wr/src/optimization.ts (2)
OptNode(18-22)precompute(72-152)libs/gi/upopt/src/markov-tree/markov.types.ts (1)
Objective(23-27)libs/gi/consts/src/artifact.ts (1)
allSubstatKeys(156-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call-build / build
- GitHub Check: typecheck
- GitHub Check: lint
🔇 Additional comments (4)
libs/gi/upopt/src/upOpt.test.ts (1)
31-56: Mixture mean/variance check is sound.The E and Var for a mixture are computed correctly (Σ p μ, Σ p(σ²+μ²)−μ_mix²). Nice sanity net for expansions.
libs/gi/upopt/src/markov-tree/makeObjective.ts (1)
32-48: LGTM — derivative stacking and zeroDeriv look correct.Index math aligns with nonzeroDerivs; zeroDeriv filters substats absent from all rows. Sweet.
libs/gi/upopt/src/markov-tree/mathUtil.ts (1)
112-120: Range endpoint is already inclusive — good.No change; keeping for posterity so future me doesn’t “fix” it at 3 AM again.
libs/gi/upopt/src/expandRolls.ts (1)
19-20: <!-- [scratchpad_start]
[task verification_getSubstatValue_signature COMPLETE]
[observations]
- Full GI getSubstatValue signature found (lines 77-82):
- params: (substatKey, rarity = 5, type: 'max'|'min'|'mid' = 'max', percent = true)
- Call in expandRolls.ts:19 is: getSubstatValue(key, rarity, 'max', false)
- Arg mapping:
- key → substatKey ✓
- rarity → rarity ✓
- 'max' → type: valid value ('max'|'min'|'mid') ✓
- false → percent: valid, sets percent=false (returns raw value not percent) ✓
[analysis]- ARG ORDER: Correct (matches function signature)
- TIER VALIDITY: 'max' IS the correct tier for GI (not 'high'/'med'/'low' which are for SR)
- 4TH PARAMETER: Valid - percent=false means return raw substat value, not percentage
- REVIEW ASSUMPTION: Reviewer may have confused GI and SR signatures, or assumed 'max' wasn't valid
- CONCLUSION: Original review comment is INCORRECT - the code is fine
[done]
All concerns verified and resolved. Code is correct.
[scratchpad_start] -->
cracks knuckles while bleary-eyed at 3 AM, gacha ad banner loading in another tab
Yo, so I actually dove into this and... the original review comment is barking up the wrong tree here.
The complete signature for GI's
getSubstatValueis:export function getSubstatValue( substatKey: SubstatKey, rarity: ArtifactRarity = 5, type: 'max' | 'min' | 'mid' = 'max', percent = true ): numberYour call at line 19:
getSubstatValue(key, rarity, 'max', false)Is 100% correct:
keymaps tosubstatKey✓raritymaps torarity✓'max'is a valid tier for GI (not SR which uses 'low'|'med'|'high') ✓falsesetspercent=falseto return raw values instead of percentages ✓Argument order is solid, tier is right for Genshin, and that fourth parameter is doing exactly what it should. The review concern seems to have mixed up the GI and SR function signatures or made wrong assumptions about what
'max'represents.Likely an incorrect or invalid review comment.
lantua
left a comment
lantua
left a comment
There was a problem hiding this comment.
We will fix it in post XD.
This PR is intended to merge the math in first without connecting anything to UI yet.
Also, please check my config files. I don't know what I'm doing.
Theoretical Idea
Restructures artifact statistics code to be more general & support requested features like reshaping / defining / unactivated substats. We set up an "artifact statsistic query" as a tree of probability nodes with edge weights describing the probability of a branch & node values being the expected function evaluation at that node.
The artifact upgrade tree is sectioned into 3 "layers" which I'm calling SubstatLevel, RollsLevel, and ValuesLevel. The children of a SubstatLevel node is a sequence of RollsLevel nodes, and the children of the Rolls level are the Values level. Intuitively, we can think of a Substat node as a selection of substats
[atk, def%, critRate%, hp]and its children as distributing the a number of rolls into each substat. For example:Then the children of the Rolls node assigns actual roll values to each substat depending on how many rolls each stat should have. Example:
So each node defines a distribution through the distribution of its children, and the distribution of the "root" node is the distribution of the possible upgrade outcomes of an artifact or process. However, because the child distribution of the Substat and Rolls nodes are known, we can estimate the mean and variance of these nodes without having to expand any of their children. This allows us to estimate the statistics of a node directly, shortcutting the need to evaluate its children.
In practice, I intend initial distribution of a "root" node to just be its direct children (a list of Substat nodes). The root node's statistics can be estimated by evaluating at each substat node; think of the initial tree as a 1-depth tree whose leaves are all of the root's children. Then the root's estimated distribution can be refined by expanding a Substat node into a sequence of Rolls nodes or Rolls into Values; generally the "root"s value will be a sum over its leaves.
Description of code
This library is centered on the probability nodes
SubstatLevelNode,RollsLevelNodeandValuesLevelNode. The objective function is encoded in anObjectivetype.Each node type has supporting code to estimate its stat distribution (mean & covariance). Using this Gaussian approximation, we can evaluate the Objective on each node to shortcut its children. Tests were written to ensure that the Gaussian's mean and covariance do actually match the child nodes' mean and covariance, as well as many intermediate steps along the way.
Exposed API (Not finished)
I intend to expose the following functions that the UI code can call:
Tree construction functions in
upOpt.ts, for the different types of artifact statistics queries.levelUpis the current upOpt feature.freshArtconsiders a new artifact from a Domain or a Strongbox.dustReshapeandelixirDefinitionare the reshaping and crafting features, respectively.Node estimation and expansion:
Issue or discord link
Testing/validation
Tests written:
makeObjective: Test linear & nonlinear objective functions, ensure the function & its derivatives are computed correctly.evaluateGaussianDegen: Test EvaluateGaussian on a 0-variance Gaussian (essentially same as previous)evaluateGaussian: Test EvaluateGaussian with non-zero covariance returns correct valuesexpandSubstatReshape: Check thatexpandSubstatwhen reshape affixes are specified returns the correct distribution of rolls.expandRolls: expand a Rolls node and ensure thatevaluateGaussianexactly matches between the node and its children for a linear objective.expandSubstat: expand a Substat node and ensure thatevaluateGaussianexactly matches between the node and its children for a linear objective.expandSubstatFancy: Same as above but with baseRolls and reshape affixesdeduplicate: Combine functionally identical child nodes (e.g. when two nodes have different substats, but neither substat appears in objective)substatProbs: Check that substat probability function matches my spreadsheet valuesChecklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit