fix(mcda): Fix forced normalization in mcda#1247
Conversation
WalkthroughAdds optional forced min-max normalization and optional prevention of value inversion to MCDA layer calculations; forces min-max when using sentiment colors in style generation; updates text layer spec to call MCDA calculation with explicit flags based on layer normalization. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Renderer
participant S as createMCDAStyle
participant L as linearNormalization
participant C as calculateMCDALayer
R->>S: build style(config)
S->>S: forceMinMaxForLayerStyle = (config.colors.type === 'sentiments')
S->>L: linearNormalization(layers, forceMinMaxForLayerStyle)
loop each layer
L->>C: calculateMCDALayer(layer, forceMinMaxInLayerStyle=forceMinMaxForLayerStyle)
Note over C: normalize if max-min or (layerStyle && forced)
C-->>L: per-layer expression
end
L-->>S: combined MCDA expression
S-->>R: style expression
sequenceDiagram
autonumber
participant T as createTextLayerSpecification
participant C as calculateMCDALayer
T->>T: if mcdaMode !== 'score'
loop each text layer
T->>C: calculateMCDALayer(layer, false, preventValueInversion=(layer.normalization === 'no'))
Note over C: orientation respects preventValueInversion
C-->>T: layer value
end
T-->>T: build text spec
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (1)
⏰ 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). (2)
🔇 Additional comments (2)
✨ 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 (
|
|
Bundle size diff
|
|
Preview environments for this PR:
These previews are automatically updated with each commit. Note: After a new deployment, it may take a few minutes for the changes to propagate and for caches to update. During this time, you might experience temporary loading issues or see an older version of the app. If the app fails to load, please wait a few minutes and try again. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1247 +/- ##
==========================================
+ Coverage 12.03% 12.04% +0.01%
==========================================
Files 724 724
Lines 29798 29817 +19
Branches 1398 1398
==========================================
+ Hits 3585 3591 +6
- Misses 25657 25670 +13
Partials 556 556
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 86 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: konturio/disaster-ninja-fe-esmAssets Changed:
Files in
Files in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (1)
185-186: absoluteMax aggregation uses Math.min; should be Math.max.Otherwise the allowed range can shrink incorrectly on the upper bound.
- acc[1] = Math.min(acc[1], range[1]); + acc[1] = Math.max(acc[1], range[1]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (3)
src/core/logical_layers/renderers/MultivariateRenderer/helpers/createTextLayerSpecification.ts(1 hunks)src/core/logical_layers/renderers/stylesConfigs/mcda/calculations/index.ts(3 hunks)src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/core/logical_layers/renderers/MultivariateRenderer/helpers/createTextLayerSpecification.ts (1)
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (1)
calculateMCDALayer(20-25)
src/core/logical_layers/renderers/stylesConfigs/mcda/calculations/index.ts (1)
src/core/logical_layers/renderers/stylesConfigs/mcda/types.ts (1)
MCDALayer(27-43)
⏰ 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: build-and-publish / test
- GitHub Check: deploy
- GitHub Check: test
🔇 Additional comments (6)
src/core/logical_layers/renderers/MultivariateRenderer/helpers/createTextLayerSpecification.ts (1)
31-31: Correctly suppressing inversion for non-normalized text.Passing preventValueInversion when normalization === 'no' matches the intent and new API. Looks good.
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (2)
74-78: Forwarding forceMinMax to per-layer calculations is correct.This ensures consistent normalization behavior across layers when required.
65-65: linearNormalization call sites verifiedI’ve located all external invocations of
linearNormalizationand confirmed they rely on the newforceMinMaxdefault (false):• src/utils/multivariate/multivariateStyle.ts:
– line 103:linearNormalization(score.layers)
– line 104:linearNormalization(base.layers)
• src/core/logical_layers/renderers/stylesConfigs/multivariate/multivariateDimensionToScore.ts:
– line 5:return linearNormalization(axis.config.layers);Since none of these calls explicitly pass a second argument, they’ll all use the default
forceMinMax = false, matching the intended behavior. No further updates are required.src/core/logical_layers/renderers/stylesConfigs/mcda/calculations/index.ts (3)
181-185: API extension is clean.New parameters (forceMinMaxInLayerStyle?, preventValueInversion?) provide needed control without breaking existing callers.
254-257: Normalization gating is correct.Min-max applies when explicitly configured or when layerStyle forces it (e.g., sentiments). Matches PR goal.
263-266: Inversion gating matches intended behavior.Prevents inverting non-normalized values in view and allows opt-out via preventValueInversion for layerStyle (used by text). Good.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes