fix the calculation of splitting factors for idcc process #210
Conversation
WalkthroughThe pull request introduces modifications to several Java classes within the CSE (Central South Europe) import runner application. The primary change involves updating the method signature of Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/Ntc.java (1)
54-60: LGTM! Consider adding documentation for the calculation formula.The refactoring improves encapsulation and code organization. However, the complex calculation of splitting factors would benefit from documentation explaining the business logic and mathematical formula.
Consider adding a documentation block explaining:
- The purpose of splitting factors
- The mathematical formula used
- The significance of merchant lines in the calculation
- Any assumptions or edge cases
Example:
+ /** + * Computes reduced splitting factors for each country. + * + * The formula used is: (countryNtc - countryMerchantFlow) / (totalNtc - totalMerchantFlow) + * where: + * - countryNtc: NTC value for a specific country + * - countryMerchantFlow: Flow on merchant lines for that country + * - totalNtc: Sum of all country NTC values + * - totalMerchantFlow: Sum of all merchant line flows + * + * @return Map of country codes to their computed splitting factors + */ public Map<String, Double> computeReducedSplittingFactors() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cse-cc-import-runner-app/src/main/java/com/farao_community/farao/cse/import_runner/app/CseData.java(1 hunks)cse-cc-import-runner-app/src/main/java/com/farao_community/farao/cse/import_runner/app/dichotomy/NetworkShifterProvider.java(1 hunks)cse-cc-import-runner-app/src/main/java/com/farao_community/farao/cse/import_runner/app/services/TtcResultService.java(1 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/Ntc.java(1 hunks)cse-lib/data/src/test/java/com/farao_community/farao/cse/data/ntc/NtcFilesTest.java(2 hunks)
🔇 Additional comments (5)
cse-cc-import-runner-app/src/main/java/com/farao_community/farao/cse/import_runner/app/dichotomy/NetworkShifterProvider.java (1)
53-53: Verify the impact of parameter removal on both D2CC and IDCC processes
The removal of ntcsByEic parameter from getReducedSplittingFactors() affects both process types. While the change looks correct, we should verify that splitting factors are still calculated correctly for both scenarios.
✅ Verification successful
Parameter removal from getReducedSplittingFactors() is safe and correct
The verification shows that the removal of ntcsByEic parameter is safe because:
getReducedSplittingFactors()in CseData now correctly computes reduced splitting factors from Ntc without needing NTC values- Both D2CC and IDCC dispatchers still receive and use the splitting factors properly:
- CseD2ccShiftDispatcher receives them as
reducedSplittingFactors - CseIdccShiftDispatcher receives them as
splittingFactors
- CseD2ccShiftDispatcher receives them as
- Test coverage exists for both dispatcher types validating the splitting factors behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of getReducedSplittingFactors to ensure consistent parameter removal
rg -l "getReducedSplittingFactors"
# Search for test coverage of both process types
rg -A 5 "ProcessType\.(D2CC|IDCC).+getReducedSplittingFactors"
Length of output: 517
Script:
#!/bin/bash
# Let's check the implementation and usage of getReducedSplittingFactors in CseData
ast-grep --pattern 'getReducedSplittingFactors($$$) { $$$ }'
# Check how the splitting factors are used in NetworkShifterProvider
rg -A 10 "splittingFactors" cse-cc-import-runner-app/src/main/java/com/farao_community/farao/cse/import_runner/app/dichotomy/NetworkShifterProvider.java
# Check the test coverage for NetworkShifterProvider
fd NetworkShifterProvider.*Test
Length of output: 964
Script:
#!/bin/bash
# Let's check the implementation in CseData class
rg -A 10 "getReducedSplittingFactors" cse-cc-import-runner-app/src/main/java/com/farao_community/farao/cse/import_runner/app/CseData.java
# Check the implementation of both dispatcher types to see how they use splitting factors
rg -A 10 "class CseD2ccShiftDispatcher"
rg -A 10 "class CseIdccShiftDispatcher"
# Check if there are any tests validating the splitting factors behavior
rg -l "splittingFactors.*[DI]2CC"
Length of output: 8339
cse-lib/data/src/test/java/com/farao_community/farao/cse/data/ntc/NtcFilesTest.java (2)
Line range hint 31-42: LGTM! Improved exception handling
Good improvement in exception handling by wrapping low-level exceptions in a domain-specific one.
58-63: Verify the updated splitting factor values
The new splitting factor values sum to 0.999 which is correct (accounting for rounding). The distribution across countries (FR: 46.6%, CH: 40.9%, AT: 3.5%, SI: 8.9%) appears to align with geographical and electrical topology expectations.
cse-cc-import-runner-app/src/main/java/com/farao_community/farao/cse/import_runner/app/services/TtcResultService.java (1)
62-62: LGTM! Consistent with the parameter removal pattern
The change aligns with the removal of ntcsByEic parameter from getReducedSplittingFactors across the codebase. The TTC result generation should now work consistently for both process types.
✅ Verification successful
Verified: Parameter removal is safe and consistent
The codebase analysis confirms that:
- The
getReducedSplittingFactorsimplementation inNtc.javahas been updated to handle both process types internally - All references to
ntcsByEicwith splitting factors have been properly removed - The TTC result generation is covered by tests in both the app and lib modules
- The remaining uses of
ntcsByEicare in different contexts (initial shifts, dichotomy) and don't interact with splitting factors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify TTC result generation test coverage
rg -l "class.*Test.*TtcResult"
# Check if there are any remaining direct uses of ntcsByEic with splitting factors
rg "ntcsByEic.*splitting[Ff]actors|splitting[Ff]actors.*ntcsByEic"
Length of output: 101
Script:
#!/bin/bash
# Let's verify the changes more thoroughly
# Check for any test files related to TTC or splitting factors
rg -l "TtcResult|SplittingFactor" --type java
# Look for the implementation of getReducedSplittingFactors
ast-grep --pattern 'getReducedSplittingFactors($$$)'
# Check for any remaining references to ntcsByEic in the codebase
rg "ntcsByEic" --type java
Length of output: 8536
cse-cc-import-runner-app/src/main/java/com/farao_community/farao/cse/import_runner/app/CseData.java (1)
45-50: Breaking change: Method signature updated to remove parameter.
The refactoring improves encapsulation by using internal state instead of requiring external NTC data. However, this is a breaking change that might affect existing clients.
Let's verify the impact on API clients:
✅ Verification successful
Let me verify the git history to see if this is a new method or a modification of an existing one.
Let me also check for any test files that might show the usage pattern:
Let me try a different approach to find test files:
Based on the results from our searches, I can generate the final response:
Breaking change appears to be safe to proceed with
The method signature change in getReducedSplittingFactors() is only used internally within the same codebase, and all existing calls found are already using the parameter-less version:
- In TtcResultService:
cseData.getReducedSplittingFactors() - In NetworkShifterProvider:
cseData.getReducedSplittingFactors()
The refactoring improves encapsulation by relying on internal state via getNtc().computeReducedSplittingFactors() and maintains the lazy initialization pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to getReducedSplittingFactors with parameters
rg -A 2 "getReducedSplittingFactors\([^)]*\)" --type java
Length of output: 1982
Script:
#!/bin/bash
# Check git history for the method
git log -L :getReducedSplittingFactors:cse-cc-import-runner-app/src/main/java/com/farao_community/farao/cse/import_runner/app/CseData.java
Length of output: 0
Script:
#!/bin/bash
# Search for test files containing getReducedSplittingFactors
rg "getReducedSplittingFactors" --type java "*Test.java"
Length of output: 107
Script:
#!/bin/bash
# Find test files first, then search within them
fd "Test\.java$" | xargs rg "getReducedSplittingFactors"
# Also check for the implementation of computeReducedSplittingFactors in Ntc class
ast-grep --pattern 'computeReducedSplittingFactors() {
$$$
}'
Length of output: 122
Please check if the PR fulfills these requirements (please use
'[x]'to check the checkboxes, or submit the PR and then click the checkboxes)Does this PR already have an issue describing the problem ? If so, link to this issue using
'#XXX'and skip the restWhat kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
(if any of the questions/checkboxes don't apply, please delete them entirely)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor