Add stock flow datasheets to external program#36
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughExternal data processing now loads and rebuilds multiple StockFlowTransformer structures (flow pathways, multipliers, spatial/lateral multipliers, orders, stock limits, transition multipliers) with conditional spatial handling. Many StockFlowTransformer fields and helper methods were widened from private to internal; FillOutputFilterFlows was replaced by FillFlowOrders. Changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/RuntimeSF/StockFlowTransformer.Collections.cs (1)
633-651: Remove assert that blocks re-entry via ExtProc; re-create the couplet map instead.Because ExtProc now calls FillFlowPathways during runtime, m_LateralFlowCoupletMap may already be set. The current Debug.Assert forces it to be null and will fail in DEBUG builds after the first load.
Apply this diff:
- Debug.Assert(this.m_LateralFlowCoupletMap == null); - - this.m_LateralFlowCoupletMap = new LateralFlowCoupletMap(); + // Re-create the couplet map on rebuild + this.m_LateralFlowCoupletMap = new LateralFlowCoupletMap();
🧹 Nitpick comments (4)
src/RuntimeSF/StockFlowTransformer.Multipliers.cs (1)
15-27: Prefer explicit failure over returning null in GetFlowMultiplierType.Returning null after Debug.Assert(false) risks an opaque NRE downstream. Consider throwing an ArgumentException with the id for clearer diagnostics.
Apply this diff:
- Debug.Assert(false); - return null; + Debug.Assert(false); + throw new ArgumentException($"Unknown FlowMultiplierTypeId: {id}");src/Runtime/STSimTransformer.ExtProc.cs (2)
401-404: Duplicate Clear() on m_FlowMultipliersByStock.Back-to-back Clear() calls are redundant.
Apply this diff:
- stockFlowTransformer.m_FlowMultipliersByStock.Clear(); - stockFlowTransformer.m_FlowMultipliersByStock.Clear(); + stockFlowTransformer.m_FlowMultipliersByStock.Clear();
377-398: DRY opportunity: factor repeated “clear/fill/rebind/recreate” pattern.These four branches share identical per-type-map rebuild logic. Consider an internal helper on StockFlowTransformer (e.g., RebuildFlowMultiplierMaps(Action clear, Action fill, Action init, Func<FlowMultiplierType, Action> clearTypeMap, Action<FlowMultiplierType, FlowMultiplierBase> addToType, Action createMap)) or expose CreateMultiplierTypeMaps() and reuse it where applicable.
Also applies to: 399-421, 422-447, 448-473
src/RuntimeSF/StockFlowTransformer.Maps.cs (1)
11-16: Encapsulation: prefer reset methods over exposing mutable fields.Directly nulling internal fields from other classes couples callers to representation. Consider internal Reset* methods (e.g., ResetStockLimitMap(), ResetFlowOrderMap()) that encapsulate null-and-create, keeping fields private.
Example:
- internal StockLimitMap m_StockLimitMap; + private StockLimitMap m_StockLimitMap; ... - internal void CreateStockLimitMap() + internal void ResetStockLimitMap() { - Debug.Assert(this.m_StockLimitMap == null); + this.m_StockLimitMap = null; this.m_StockLimitMap = new StockLimitMap(this.ResultScenario, this.m_StockLimits); }Then call Reset* from ExtProc instead of setting fields to null.
Also applies to: 21-38, 45-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/Runtime/STSimTransformer.ExtProc.cs(2 hunks)src/RuntimeSF/StockFlowTransformer.Collections.cs(9 hunks)src/RuntimeSF/StockFlowTransformer.Initialize.cs(2 hunks)src/RuntimeSF/StockFlowTransformer.Maps.cs(2 hunks)src/RuntimeSF/StockFlowTransformer.Multipliers.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Runtime/STSimTransformer.ExtProc.cs (5)
src/RuntimeSF/StockFlowTransformer.Collections.cs (9)
StockFlowTransformer(14-1011)FillFlowPathways(633-651)FillFlowMultipliers(653-779)FillFlowMultipliersByStock(419-527)FillFlowSpatialMultipliers(894-951)FillFlowLateralMultipliers(953-1010)FillFlowOrders(853-892)FillStockLimits(355-417)FillStockTransitionMultipliers(529-631)src/RuntimeSF/StockFlowTransformer.Initialize.cs (3)
StockFlowTransformer(13-394)InitializeFlowMultiplierDistributionValues(106-125)InitializeFlowMultiplierByStockDistributionValues(130-146)src/RuntimeSF/StockFlowTransformer.Maps.cs (5)
StockFlowTransformer(9-85)CreateFlowPathwayMap(33-37)CreateFlowOrderMap(45-49)CreateStockLimitMap(21-25)CreateStockTransitionMultiplierMap(27-31)src/RuntimeSF/StockFlowTransformer.Multipliers.cs (4)
StockFlowTransformer(13-206)FlowMultiplierType(15-27)ValidateFlowSpatialMultipliers(171-187)ValidateFlowLateralMultipliers(189-205)src/RuntimeSF/FlowMultiplierType.cs (12)
ClearFlowMultiplierMap(115-119)AddFlowMultiplier(75-83)CreateFlowMultiplierMap(139-148)ClearFlowMultiplierByStockMap(133-137)AddFlowMultiplierByStock(105-113)CreateFlowMultiplierByStockMap(172-181)ClearFlowSpatialMultiplierMap(121-125)AddFlowSpatialMultiplier(85-93)CreateSpatialFlowMultiplierMap(150-159)ClearFlowLateralMultiplierMap(127-131)AddFlowLateralMultiplier(95-103)CreateLateralFlowMultiplierMap(161-170)
src/RuntimeSF/StockFlowTransformer.Maps.cs (1)
src/RuntimeSF/LateralFlowCoupletMap.cs (1)
LateralFlowCoupletMap(10-57)
src/RuntimeSF/StockFlowTransformer.Collections.cs (11)
src/RuntimeSF/FlowMultiplierTypeCollection.cs (1)
FlowMultiplierTypeCollection(8-10)src/RuntimeSF/InitialStockNonSpatialCollection.cs (1)
InitialStockNonSpatialCollection(8-14)src/Runtime/STSimTransformer.Spatial.cs (3)
Dictionary(302-341)Dictionary(349-377)Dictionary(766-818)src/RuntimeSF/StockLimitCollection.cs (1)
StockLimitCollection(8-10)src/RuntimeSF/FlowMultiplierByStockCollection.cs (1)
FlowMultiplierByStockCollection(8-10)src/RuntimeSF/StockTransitionMultiplierCollection.cs (1)
StockTransitionMultiplierCollection(8-10)src/RuntimeSF/FlowPathwayCollection.cs (1)
FlowPathwayCollection(8-10)src/RuntimeSF/FlowMultiplierCollection.cs (1)
FlowMultiplierCollection(8-10)src/RuntimeSF/FlowSpatialMultiplierCollection.cs (1)
FlowSpatialMultiplierCollection(8-10)src/RuntimeSF/FlowLateralMultiplierCollection.cs (1)
FlowLateralMultiplierCollection(8-10)src/RuntimeSF/OutputFilterCollection.cs (1)
OutputFilterCollection(8-34)
🔇 Additional comments (14)
src/RuntimeSF/StockFlowTransformer.Initialize.cs (2)
106-125: Visibility widened is appropriate; no behaviour change.Making InitializeFlowMultiplierDistributionValues internal enables ExtProc to re-init distributions after external loads. Looks good.
130-146: Visibility widened is appropriate; mirrors flow multipliers.InitializeFlowMultiplierByStockDistributionValues being internal is consistent with its new call site in ExtProc. All good.
src/RuntimeSF/StockFlowTransformer.Multipliers.cs (1)
171-205: Exposing validations is sensible for ExtProc rebuild flow.Internalising ValidateFlowSpatialMultipliers and ValidateFlowLateralMultipliers aligns with their new usage. No issues found.
src/Runtime/STSimTransformer.ExtProc.cs (7)
252-253: Minor: local alias improves readability.Using stockFlowTransformer reduces noise in this method. Good call.
377-398: Flow multipliers: rebuild sequence is correct.Clear → Fill → Initialize distributions → clear per-type maps → rebind → recreate maps matches existing init patterns.
399-421: By-stock multipliers: sequence mirrors flow multipliers.After removing the duplicate Clear(), the flow is consistent and correct.
422-447: Spatial multipliers: guarded by IsSpatial and validated — good.Clear rasters, refill, validate, then rebuild per-type maps is the right order.
448-473: Lateral multipliers: rebuild pattern is consistent.Matches spatial handling; no issues spotted.
474-480: Flow orders: full rebuild is straightforward.Clear, fill, reset map to null, recreate — good.
481-487: Stock limits: full rebuild is straightforward.No concerns.
src/RuntimeSF/StockFlowTransformer.Maps.cs (1)
11-16: Internalising map fields/factories enables ExtProc rebuilds.Access widening is consistent with the new external data pathway and keeps APIs non-public. Fine by me.
Also applies to: 21-38, 45-49
src/RuntimeSF/StockFlowTransformer.Collections.cs (3)
20-21: Access widening for collections is acceptable, but watch invariants.Internal readonly retains reference stability while allowing Clear()/Add(). Given the new usage from ExtProc, this is reasonable.
Also applies to: 24-36
653-779: Flow multipliers fill path unchanged; matches rebuild expectations.Parsing, validation and IsDisabled handling look consistent with existing logic.
355-417: Other Fill methods internalised for ExtProc use — LGTM.*The data parsing/validation and raster handling are consistent; memory conservation via filename-keyed raster caches is maintained.
Also applies to: 419-527, 529-631, 853-892, 894-951, 953-1010
Summary by CodeRabbit
New Features
Bug Fixes