Skip to content

Kb ext program datasheets#38

Merged
katieb1 merged 2 commits intomainfrom
kb-ext-program-datasheets
Oct 9, 2025
Merged

Kb ext program datasheets#38
katieb1 merged 2 commits intomainfrom
kb-ext-program-datasheets

Conversation

@katieb1
Copy link
Member

@katieb1 katieb1 commented Sep 30, 2025 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Bug Fixes
    • Ensures user-defined distributions are consistently applied across data import and initialization, including transition targets, multiplier values, state attribute values, transition attribute values, and transition attribute targets.
    • Prevents defaults from overriding configured distributions during setup.
    • Improves consistency and predictability of simulation results when importing or updating these data sheets.

@katieb1 katieb1 requested a review from alexembrey September 30, 2025 17:30
@coderabbitai
Copy link

coderabbitai bot commented Sep 30, 2025 •
edited
Loading

Walkthrough

Inserted NormalizeForUserDistributions calls across multiple data-ready paths to apply user-defined distributions before subsequent Initialize... steps for transition targets, transition multiplier values, state attribute values, transition attribute values, and transition attribute targets.

Changes

Cohort / File(s) Summary of Changes
Normalization insertions in data-ready paths
src/Runtime/STSimTransformer.ExtProc.cs		 Added NormalizeForUserDistributions calls: after FillTransitionTargetCollection (TRANSITION_TARGET), after FillTransitionMultiplierValueCollection (TRANSITION_MULTIPLIER_VALUE), before initializing StateAttributeValues (STATE_ATTRIBUTE_VALUE), before initializing TransitionAttributeValues (TRANSITION_ATTRIBUTE_VALUE), and after filling TransitionAttributeTargetCollection (TRANSITION_ATTRIBUTE_TARGET); normalization now precedes Initialize... calls.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title “Kb ext program datasheets” is too generic and does not clearly summarise the addition of normalization steps for user distributions in STSimTransformer external processing datasheet paths, making it hard to understand the primary change at a glance. Consider renaming the pull request to clearly reflect the key change, for example “Normalize user distributions in STSimTransformer datasheet loading paths” to convey the main purpose concisely.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch kb-ext-program-datasheets
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c8eaddf and 5b2bd5c.

📒 Files selected for processing (1)
  • src/Runtime/STSimTransformer.ExtProc.cs (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Runtime/STSimTransformer.ExtProc.cs (3)
src/Runtime/STSimTransformer.Initialize.cs (1)
  • InitializeTransitionAttributes (552-569)
src/Shared/Strings.cs (1)
  • Strings (8-700)
src/Runtime/STSimTransformer.Collections.cs (1)
  • FillTransitionAttributeTargetCollection (1737-1839)
🔇 Additional comments (5)
src/Runtime/STSimTransformer.ExtProc.cs (5)

260-268: LGTM! Normalization correctly placed before initialization.

The call to NormalizeForUserDistributions() is appropriately positioned after filling the transition targets collection and before initializing distribution values and prioritizations.

269-291: LGTM! Normalization correctly placed before initialization.

The call to NormalizeForUserDistributions() is appropriately positioned after filling the transition multiplier values and before initializing distribution values. The subsequent map rebuilding operations will work with the normalized data.

348-356: LGTM! Normalization correctly placed before initialization.

The call to NormalizeForUserDistributions() is appropriately positioned after clearing and filling the state attribute values collection and before calling InitializeStateAttributes(), which will rebuild the nulled maps with normalized data.

357-365: LGTM! Normalization correctly placed before initialization.

The call to NormalizeForUserDistributions() is appropriately positioned after clearing and filling the transition attribute values collection and before calling InitializeTransitionAttributes(), which will rebuild the TransitionAttributeValueMap with normalized data.

366-374: LGTM! Normalization correctly placed before initialization.

The call to NormalizeForUserDistributions() is appropriately positioned after filling the transition attribute targets collection and before initializing distribution values and prioritizations. This mirrors the pattern used for transition targets at lines 260-268.

Comment @coderabbitai help to get the list of available commands and usage tips.

alexembrey
alexembrey approved these changes Sep 30, 2025
View reviewed changes
Copy link
Collaborator

@alexembrey alexembrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is similar to the previous fix for multipliers? If so, it looks good to me...

@katieb1 katieb1 merged commit a55e6ba into main Oct 9, 2025
1 check passed
@katieb1 katieb1 deleted the kb-ext-program-datasheets branch October 9, 2025 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexembrey alexembrey alexembrey approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@katieb1 @alexembrey