NTCAnnual values as backup when periods are missing from NTCRed file #217
Conversation
WalkthroughThe pull request introduces changes to the NTC (Net Transfer Capacity) data handling classes in the CSE (Central Western European) library. The primary modification is the transition from returning Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 1
🔭 Outside diff range comments (1)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocumentAdapted.java (1)
Line range hint
37-42: Consider enhancing error handling for null TNTC.The current implementation might throw a NullPointerException if
getTNtcFromLineFromYearlyNtcreturns null, as there's no null check before accessingtNtc.getType().Consider this safer approach:
- TNTC tNtc = NtcUtilAdapted.getTNtcFromLineFromYearlyNtc(targetDateTime, tLine); - if (tNtc.getType() == null || tNtc.getType().equalsIgnoreCase(NtcUtil.ABSOLUTE)) { - return new LineInformation(tLine.getCNtc().value(), NtcUtil.ABSOLUTE, tNtc.getV().doubleValue()); - } - throw new CseDataException("Flow for yearly value must be absolute"); + TNTC tNtc = NtcUtilAdapted.getTNtcFromLineFromYearlyNtc(targetDateTime, tLine); + if (tNtc == null) { + throw new CseDataException("No NTC values found for line: " + tLine.getCode()); + } + if (tNtc.getType() == null || tNtc.getType().equalsIgnoreCase(NtcUtil.ABSOLUTE)) { + return new LineInformation(tLine.getCNtc().value(), NtcUtil.ABSOLUTE, tNtc.getV().doubleValue()); + } + throw new CseDataException("Flow for yearly value must be absolute");
🧹 Nitpick comments (8)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/Ntc.java (2)
115-118: Enhance error message with context.While the error handling is good, the message could be more specific about the context where the line information is missing (daily or yearly NTC).
Consider updating the error message:
- .orElseThrow(() -> new CseInternalException(String.format("No information available for line %s", lineId))); + .orElseThrow(() -> new CseInternalException(String.format("No daily NTC information available for line %s", lineId)));
160-175: Remove redundant Optional.get() calls.Inside the
ifPresentblock, theLineInformationis already unwrapped, making theget()calls redundant and potentially confusing.Consider this improvement:
private static Map<String, Double> getFlowPerLineId(Map<String, LineInformation> yearlyLinePerId, Map<String, Optional<LineInformation>> dailyLinePerId) { Map<String, Double> flowPerLine = yearlyLinePerId.entrySet().stream() .collect(Collectors.toMap( Map.Entry::getKey, entry -> entry.getValue().getFlow() )); dailyLinePerId.forEach((lineId, lineInformation) -> lineInformation.ifPresent(line -> { if (line.getVariationType().equalsIgnoreCase(NtcUtil.ABSOLUTE)) { - flowPerLine.put(lineId, lineInformation.get().getFlow()); + flowPerLine.put(lineId, line.getFlow()); } else { double initialFlow = Optional.ofNullable(flowPerLine.get(lineId)).orElse(0.); - flowPerLine.put(lineId, initialFlow + lineInformation.get().getFlow()); + flowPerLine.put(lineId, initialFlow + line.getFlow()); } }) ); return flowPerLine; }cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocument.java (1)
38-43: Simplify Optional handling and add validation.While the Optional handling is functionally correct, there are several improvements that could be made:
- Avoid multiple
.get()calls on the same Optional- Add validation for
CNtcvalue- Simplify the nested Optional mapping
Consider this refactoring:
- Optional<TNTC> optionalTntc = NtcUtil.getTNtcFromLineFromNtcRedFile(targetDateTime, tLine); - if (optionalTntc.isPresent()) { - return Optional.of(new LineInformation(tLine.getCNtc().value(), optionalTntc.get().getType(), optionalTntc.get().getV().doubleValue())); - } else { - return Optional.empty(); - } + return NtcUtil.getTNtcFromLineFromNtcRedFile(targetDateTime, tLine) + .map(tNtc -> { + Objects.requireNonNull(tLine.getCNtc(), "CNtc must not be null"); + return new LineInformation( + tLine.getCNtc().value(), + tNtc.getType(), + tNtc.getV().doubleValue() + ); + });cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtilAdapted.java (2)
27-31: LGTM! Consider simplifying the implementation.The change to return
Optional<TNTC>improves error handling and null safety. However, the implementation could be simplified.Consider this more concise implementation:
- static Optional<TNTC> getTNtcFromLineFromNtcRedFile(OffsetDateTime targetDateTime, TLine tLine) { - return getTNtcFromPeriods(targetDateTime, tLine.getPeriod()) - .stream() - .collect(toOptional()); - } + static Optional<TNTC> getTNtcFromLineFromNtcRedFile(OffsetDateTime targetDateTime, TLine tLine) { + List<TNTC> ntcs = getTNtcFromPeriods(targetDateTime, tLine.getPeriod()); + return ntcs.isEmpty() ? Optional.empty() : Optional.of(ntcs.get(0)); + }Also consider renaming the method to something shorter like
getTNtcFromNtcRedFilesince "Line" is redundant given theTLineparameter.
Line range hint
33-37: Add documentation and improve error handling.The method provides a good fallback mechanism for yearly NTC values, but could benefit from some improvements:
- Add Javadoc explaining when this method should be used vs
getTNtcFromLineFromNtcRedFile.- Improve the error message to be more descriptive.
- Simplify the implementation.
+ /** + * Retrieves TNTC from yearly NTC values for a given line. + * Used as a fallback when NTC values are not found in the NTCRed file. + * + * @param targetDateTime the target date and time + * @param tLine the line to get NTC for + * @return TNTC value + * @throws CseDataException if no NTC definition is found in the yearly values + */ static TNTC getTNtcFromLineFromYearlyNtc(OffsetDateTime targetDateTime, TLine tLine) { - return getTNtcFromPeriods(targetDateTime, tLine.getPeriod()).stream() - .collect(toOptional()) - .orElseThrow(() -> new CseDataException(String.format("No NTC definition for line %s", tLine.getCode()))); + List<TNTC> ntcs = getTNtcFromPeriods(targetDateTime, tLine.getPeriod()); + if (ntcs.isEmpty()) { + throw new CseDataException(String.format( + "No yearly NTC definition found for line %s at %s", + tLine.getCode(), + targetDateTime + )); + } + return ntcs.get(0); }cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocumentAdapted.java (1)
Line range hint
26-44: Consider returning Optional for consistency.To align with the architectural changes mentioned in the AI summary about transitioning to Optional types, consider updating the method signature and implementation.
Here's a suggested approach:
- Map<String, LineInformation> getLineInformationPerLineId(Predicate<TLine> lineSelector) { + Map<String, Optional<LineInformation>> getLineInformationPerLineId(Predicate<TLine> lineSelector) { TSpecialLines tSpecialLines = getTNtcValues().getSpecialLines(); if (tSpecialLines != null) { return tSpecialLines.getLine().stream() .filter(lineSelector) .collect(Collectors.toMap( TLine::getCode, tLine -> { try { TNTC tNtc = NtcUtilAdapted.getTNtcFromLineFromYearlyNtc(targetDateTime, tLine); if (tNtc == null) { return Optional.empty(); } if (tNtc.getType() == null || tNtc.getType().equalsIgnoreCase(NtcUtil.ABSOLUTE)) { return Optional.of(new LineInformation(tLine.getCNtc().value(), NtcUtil.ABSOLUTE, tNtc.getV().doubleValue())); } throw new CseDataException("Flow for yearly value must be absolute"); } catch (Exception e) { return Optional.empty(); } } )); } - return Collections.emptyMap(); + return Collections.emptyMap(); // Consider returning Map<String, Optional<LineInformation>> }cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtil.java (2)
24-27: LGTM! Consider adding Javadoc.The changes improve null safety by returning
Optional<TNTC>and the renamed method clearly indicates its data source. The implementation is clean and follows functional programming practices.Consider adding Javadoc to document:
- The method's purpose
- @param descriptions
- @return value and when it might be empty
- Any exceptions that might be thrown
Line range hint
29-33: Enhance error handling and add input validation.The implementation is good and appropriately throws an exception for missing yearly NTC data. Consider these improvements:
- Add null checks for input parameters
- Enhance the error message with more context
static TNTC getTNtcFromLineFromYearlyNtc(OffsetDateTime targetDateTime, TLine tLine) { + Objects.requireNonNull(targetDateTime, "targetDateTime cannot be null"); + Objects.requireNonNull(tLine, "tLine cannot be null"); return getTNtcFromPeriods(targetDateTime, tLine.getPeriod()).stream() .collect(toOptional()) - .orElseThrow(() -> new CseDataException(String.format("No NTC definition for line %s", tLine.getCode()))); + .orElseThrow(() -> new CseDataException( + String.format("No yearly NTC definition found for line %s at %s", + tLine.getCode(), + targetDateTime + ) + ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocument.java(3 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocumentAdapted.java(3 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/Ntc.java(5 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtil.java(1 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtilAdapted.java(1 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocument.java(1 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocumentAdapted.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (11)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/Ntc.java (2)
11-11: LGTM! Import addition aligns with new error handling.The addition of
CseInternalExceptionimport supports the new error handling mechanism for missing line information.
67-67: Well-structured Optional handling implementation.The consistent implementation of
Optional<LineInformation>across methods improves null safety and error handling. The pattern is well-applied in both adapted and non-adapted code paths.Also applies to: 72-72, 85-85, 91-92, 101-101, 107-108
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocumentAdapted.java (3)
14-14: LGTM!The consolidation of java.util imports into a wildcard import is consistent with the existing codebase style.
37-42: LGTM! Robust Optional handling implementation.The new implementation properly handles missing TNTC data using Optional, making the code more robust and safer.
27-27: Verify the impact of this breaking change.The return type change from
Map<String, LineInformation>toMap<String, Optional<LineInformation>>is a breaking change that requires updates in all consuming code. Let's verify the impact:✅ Verification successful
Not a breaking change - consuming code already handles Optional return type
The change aligns with the existing pattern where daily document variants return
Optionalvalues, while yearly documents provide non-optional base information. The consuming code inNtc.javaalready correctly handles both return types in thegetFlowPerLineIdmethod.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of getLineInformationPerLineId rg -l "getLineInformationPerLineId" | while read -r file; do echo "=== $file ===" rg -A 5 "getLineInformationPerLineId" "$file" doneLength of output: 5551
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocument.java (2)
14-14: LGTM!The consolidation of utility imports is consistent with the existing import style in the file.
27-27: Verify all callers handle the Optional return type correctly.The change to return
Optional<LineInformation>is a good design choice that makes the possibility of missing data explicit. However, this is a breaking change that requires updates in all calling code.✅ Verification successful
✅ All callers correctly handle the Optional return type
The change is safe as all callers in
Ntc.javaalready handle both the Optional return type from daily documents and the non-Optional type from yearly documents appropriately. The code consistently usesMap.of()when daily documents are null and properly processes the Optional values when present.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all callers of getLineInformationPerLineId to ensure they handle Optional correctly ast-grep --pattern 'getLineInformationPerLineId($$$)' -A 5Length of output: 60
Script:
#!/bin/bash # Search for any occurrences of the method name with context rg "getLineInformationPerLineId" -B 2 -A 5Length of output: 9601
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtilAdapted.java (1)
Line range hint
27-37: Verify the backup mechanism implementation.The interaction between
getTNtcFromLineFromNtcRedFileandgetTNtcFromLineFromYearlyNtcimplements the backup mechanism mentioned in the PR objectives. Let's verify how these methods are used together:✅ Verification successful
Backup mechanism implementation is correct
The implementation properly handles the backup scenario:
- Daily NTC values are attempted first through
getTNtcFromLineFromNtcRedFile- When daily values are unavailable, yearly values from
getTNtcFromLineFromYearlyNtcare used as backup- The coordination is handled by the
Ntcclass which always requires yearly values but makes daily values optional🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find where these methods are called to understand the backup mechanism implementation # Search for usage of both methods echo "=== Searching for getTNtcFromLineFromNtcRedFile usage ===" rg "getTNtcFromLineFromNtcRedFile" -A 5 echo -e "\n=== Searching for getTNtcFromLineFromYearlyNtc usage ===" rg "getTNtcFromLineFromYearlyNtc" -A 5Length of output: 7676
Script:
#!/bin/bash # Search for where DailyNtcDocument and YearlyNtcDocument are used together echo "=== Searching for DailyNtcDocument usage ===" rg "DailyNtcDocument" -A 5 echo -e "\n=== Searching for YearlyNtcDocument usage ===" rg "YearlyNtcDocument" -A 5Length of output: 28479
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocumentAdapted.java (1)
37-37: Verify consistency with Optional return type changes.While the method name change from
getTNtcFromLinetogetTNtcFromLineFromYearlyNtcbetter reflects its specific purpose, there seems to be an inconsistency. The AI summary mentions a transition toOptional<LineInformation>returns, but this implementation still uses directTNTCreturns without Optional wrapping.Let's verify the implementation in NtcUtilAdapted:
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtil.java (1)
Line range hint
24-33: Verify the impact of API changes.The architectural changes look good, introducing clear distinction between NTC data sources with appropriate error handling strategies. However, we should verify:
✅ Verification successful
API changes have been properly integrated
The changes have been successfully integrated with:
- No references to old methods found
- Proper handling of Optional returns in NtcRedFile consumers
- Consistent error handling patterns across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential breaking changes impact # Find all references to the old method name echo "Checking for old method references..." rg "getTNtcFromLine\(" --type java # Find all current callers of the new methods echo "Checking current usage..." rg "getTNtcFromLineFromNtcRedFile|getTNtcFromLineFromYearlyNtc" --type java -A 2 # Look for potential error handling patterns echo "Checking error handling patterns..." rg "CseDataException|Optional<TNTC>" --type javaLength of output: 19138
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocument.java (1)
34-34: LGTM! Verify edge cases in the new method.The change to use
getTNtcFromLineFromYearlyNtcaligns well with the PR objective of handling NTCAnnual values. The method name is more specific and clearer about its purpose.Please verify that the new method handles these edge cases:
- Missing or null TLine data
- Invalid date-time values
- Boundary conditions for yearly data
✅ Verification successful
Implementation verified - edge cases are properly handled
The
getTNtcFromLineFromYearlyNtcimplementation includes comprehensive error handling:
- Null/missing data through Optional pattern
- Clear exceptions for missing NTC definitions
- Validation of absolute values
- Proper date-time handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test coverage of the new method rg -l "getTNtcFromLineFromYearlyNtc.*test" --type java # Look for error handling patterns ast-grep --pattern 'if ($_ == null) { $$$ }' --lang javaLength of output: 3587
Script:
#!/bin/bash # Find implementation of getTNtcFromLineFromYearlyNtc ast-grep --pattern 'getTNtcFromLineFromYearlyNtc' --lang java # Search for NtcUtil test files rg -l "NtcUtil.*Test" --type java # Look for yearly NTC error handling rg "throw.*CseDataException.*yearly" --type javaLength of output: 1637
Script:
#!/bin/bash # Show implementation of getTNtcFromLineFromYearlyNtc rg -A 10 "getTNtcFromLineFromYearlyNtc" --type java # Look for any test files related to yearly NTC rg -l "YearlyNtc.*Test" --type javaLength of output: 6341
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocumentAdapted.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocumentAdapted.java (1)
41-46:⚠️ Potential issueMissing backup mechanism using NTCAnnual values.
According to the PR objectives, this change should implement a backup mechanism using NTCAnnual values when periods are missing from the NTCRed file. However, the current implementation only returns
Optional.empty()when TNTC data is missing, without attempting to use NTCAnnual values as a backup.Consider implementing the backup mechanism like this:
Optional<TNTC> optionalTntc = NtcUtilAdapted.getTNtcFromLineFromNtcRedFile(targetDateTime, tLine); if (optionalTntc.isPresent()) { return Optional.of(new LineInformation(tLine.getCNtc().value(), optionalTntc.get().getType(), optionalTntc.get().getV().doubleValue())); } else { - return Optional.empty(); + // Try to get NTCAnnual values as backup + return NtcUtilAdapted.getTNtcFromLineFromNtcAnnualFile(targetDateTime, tLine) + .map(tntc -> new LineInformation(tLine.getCNtc().value(), tntc.getType(), tntc.getV().doubleValue())); }
🧹 Nitpick comments (1)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocument.java (1)
42-47: Add null checks for defensive programming.While the Optional handling is well implemented, the code could be more robust with null checks for the TNTC fields.
Consider adding null checks:
Optional<TNTC> optionalTntc = NtcUtil.getTNtcFromLineFromNtcRedFile(targetDateTime, tLine); if (optionalTntc.isPresent()) { - return Optional.of(new LineInformation(tLine.getCNtc().value(), optionalTntc.get().getType(), optionalTntc.get().getV().doubleValue())); + TNTC tntc = optionalTntc.get(); + if (tLine.getCNtc() == null || tntc.getType() == null || tntc.getV() == null) { + return Optional.empty(); + } + return Optional.of(new LineInformation( + tLine.getCNtc().value(), + tntc.getType(), + tntc.getV().doubleValue() + )); } else { return Optional.empty(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocument.java(3 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocumentAdapted.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocument.java (2)
18-18: LGTM! Import added for Optional usage.The import is correctly placed and necessary for the new Optional-based implementation.
31-31: Verify the impact of return type change.The method's return type has been changed from
Map<String, LineInformation>toMap<String, Optional<LineInformation>>. While this is a good practice for handling missing values, it's a breaking change that requires updates in all calling code.Let's verify all usages of this method to ensure they handle the new Optional wrapper:
✅ Verification successful
Return type change is properly handled in the codebase
The change from
Map<String, LineInformation>toMap<String, Optional<LineInformation>>inDailyNtcDocumentis part of a deliberate design where daily NTC documents handle potential missing values with Optional, while yearly documents provide guaranteed values. All callers inNtc.javaalready correctly handle both return types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of getLineInformationPerLineId ast-grep --pattern 'getLineInformationPerLineId($$$)'Length of output: 55
Script:
#!/bin/bash # Search for method name using ripgrep with context rg "getLineInformationPerLineId" -l # Then get detailed context for each file rg "getLineInformationPerLineId" -B 2 -A 2Length of output: 7335
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocumentAdapted.java (2)
18-18: LGTM! Import addition is appropriate.The addition of Optional import aligns with the new method signature changes.
31-31: Verify impact of breaking API change.The return type change from
Map<String, LineInformation>toMap<String, Optional<LineInformation>>is a breaking change that will require updates in all calling code.Let's verify the impact:
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/Ntc.java
Outdated
Show resolved
Hide resolved
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)
115-118: Consider enhancing the error message.The implementation is clean and follows functional programming principles. However, the error message could be more descriptive.
Consider this enhancement:
- .orElseThrow(() -> new CseInternalException(String.format("No information available for line %s", lineId))); + .orElseThrow(() -> new CseInternalException(String.format("No line information available in either yearly or daily NTC documents for line ID: %s", lineId)));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/Ntc.java(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/Ntc.java (3)
11-11: LGTM! Import addition is appropriate.The addition of CseInternalException import aligns with the new error handling requirements.
67-67: LGTM! Consistent use of Optional for better null safety.The transition to
Optional<LineInformation>makes the potential absence of line information explicit and type-safe, which aligns well with the PR's objective of handling missing periods.Also applies to: 72-72, 85-85, 101-101
160-175: Simplify the lambda parameter name.As mentioned in a previous review, you can simplify
lineInformation.get()to just use the lambda parameter since you're insideifPresent.Apply these enhancements:
private static Map<String, Double> getFlowPerLineId(Map<String, LineInformation> yearlyLinePerId, Map<String, Optional<LineInformation>> dailyLinePerId) { Map<String, Double> flowPerLine = yearlyLinePerId.entrySet().stream() .collect(Collectors.toMap( Map.Entry::getKey, entry -> entry.getValue().getFlow() )); dailyLinePerId.forEach((lineId, lineInformation) -> - lineInformation.ifPresent(line -> { - if (line.getVariationType().equalsIgnoreCase(NtcUtil.ABSOLUTE)) { - flowPerLine.put(lineId, line.getFlow()); - } else { - double initialFlow = Optional.ofNullable(flowPerLine.get(lineId)).orElse(0.); - flowPerLine.put(lineId, initialFlow + line.getFlow()); - } - }) + lineInformation.ifPresent(dailyLine -> { + if (dailyLine.getVariationType().equalsIgnoreCase(NtcUtil.ABSOLUTE)) { + flowPerLine.put(lineId, dailyLine.getFlow()); + } else { + double initialFlow = Optional.ofNullable(flowPerLine.get(lineId)).orElse(0.); + flowPerLine.put(lineId, initialFlow + dailyLine.getFlow()); + } + }) ); return flowPerLine; }The change renames the lambda parameter to
dailyLinefor better clarity about the data source.
| if (optionalTntc.isPresent()) { | ||
| return Optional.of(new LineInformation(tLine.getCNtc().value(), optionalTntc.get().getType(), optionalTntc.get().getV().doubleValue())); | ||
| } else { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
Is it necessary to put all line-codes in the Map, even if they are not associated with any data?
Putting in it only the elements that are associated with a real value would make the code lighter, as you wouldn't have to deal with Optionals everywhere.
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtilAdapted.java
Outdated
Show resolved
Hide resolved
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtil.java
Outdated
Show resolved
Hide resolved
| if (optionalTntc.isPresent()) { | ||
| return Optional.of(new LineInformation(tLine.getCNtc().value(), optionalTntc.get().getType(), optionalTntc.get().getV().doubleValue())); | ||
| } else { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
Same as DailyNtcDocument: https://github.com/farao-community/gridcapa-cse/pull/217/files#r1933765489
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cse-lib/data/src/test/java/com/farao_community/farao/cse/data/ntc/NtcFilesTest.java (3)
82-95: Consider enhancing test coverage and documentation.The test effectively verifies the fallback behavior when a period is missing in the NTCRed file. However, consider these improvements:
- Add a comment explaining the expected behavior and why 150 is the expected value.
- Consider adding assertions to verify the state before falling back to annual values.
@Test void testFixedFlowWhenPeriodIsMissingInTheNTcRedFile() { + // When a period is missing in the NTCRed file for the MENDRISIO_CAGNO_ID line, + // the system should fall back to the annual value of 150 OffsetDateTime targetDateTime = OffsetDateTime.parse("2021-06-24T16:30Z"); try (InputStream yearlyData = getClass().getResourceAsStream("2021_2Dp_NTC_annual_CSE1.xml"); InputStream dailyData = getClass().getResourceAsStream("20210624_2D4_NTC_reductions_CSE1_missingPeriodForSpecialLine.xml") ) { Map<String, Double> fixedFlowLines = new Ntc(new YearlyNtcDocument(targetDateTime, DataUtil.unmarshalFromInputStream(yearlyData, NTCAnnualDocument.class)), new DailyNtcDocument(targetDateTime, DataUtil.unmarshalFromInputStream(dailyData, NTCReductionsDocument.class)), false).getFlowOnFixedFlowLines(); + // Verify the map contains only the expected line + assertEquals(1, fixedFlowLines.size()); assertEquals(150, fixedFlowLines.get(MENDRISIO_CAGNO_ID), DOUBLE_PRECISION); } catch (IOException | JAXBException e) { throw new CseInvalidDataException("Impossible to create NTC", e); } }
97-111: Improve variable naming and test documentation.The test effectively verifies the error handling when a period is missing in the annual file. However, consider these improvements:
- Use a more descriptive variable name than
ntc1.- Add a comment explaining the expected behavior.
- Fix the inconsistent casing in the method name (
NTcvsNTC).@Test - void testFixedFlowThrowsExceptionWhenPeriodIsMissingInTheNTcAnnualFile() { + void testFixedFlowThrowsExceptionWhenPeriodIsMissingInTheNTCAnnualFile() { + // When a period is missing in the NTC annual file for the MENDRISIO_CAGNO_ID line, + // the system should throw a CseDataException as there is no fallback value available + OffsetDateTime targetDateTime = OffsetDateTime.parse("2021-06-24T16:30Z"); try (InputStream yearlyData = getClass().getResourceAsStream("2021_2Dp_NTC_annual_CSE1_missingPeriodsForSpecialLine.xml"); InputStream dailyData = getClass().getResourceAsStream("20210624_2D4_NTC_reductions_CSE1.xml") ) { - Ntc ntc1 = new Ntc(new YearlyNtcDocument(targetDateTime, DataUtil.unmarshalFromInputStream(yearlyData, NTCAnnualDocument.class)), + Ntc ntcWithMissingPeriod = new Ntc(new YearlyNtcDocument(targetDateTime, DataUtil.unmarshalFromInputStream(yearlyData, NTCAnnualDocument.class)), new DailyNtcDocument(targetDateTime, DataUtil.unmarshalFromInputStream(dailyData, NTCReductionsDocument.class)), false); Exception exception = assertThrows(CseDataException.class, - () -> ntc1.getFlowOnFixedFlowLines()); + () -> ntcWithMissingPeriod.getFlowOnFixedFlowLines()); assertEquals("No NTC definition for line ml_mendrisio-cagno", exception.getMessage()); } catch (IOException | JAXBException e) { throw new CseInvalidDataException("Impossible to create NTC", e); } }
81-81: Consider adding tests for additional scenarios.To ensure comprehensive test coverage of the NTC fallback mechanism, consider adding tests for:
- When both NTCRed and annual files have valid periods (happy path)
- When both files have missing periods for the same line
cse-lib/data/src/test/resources/com/farao_community/farao/cse/data/ntc/20210624_2D4_NTC_reductions_CSE1_missingPeriodForSpecialLine.xml (1)
1-509: Consider adding more test scenarios for special lines.While this file effectively tests the missing period scenario, consider adding test files for other edge cases:
- Multiple special lines with overlapping periods
- Special lines with different time intervals
- Special lines with gaps between periods
Would you like me to help generate additional test files covering these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cse-lib/data/src/test/java/com/farao_community/farao/cse/data/ntc/NtcFilesTest.java(2 hunks)cse-lib/data/src/test/resources/com/farao_community/farao/cse/data/ntc/20210624_2D4_NTC_reductions_CSE1_missingPeriodForSpecialLine.xml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
cse-lib/data/src/test/java/com/farao_community/farao/cse/data/ntc/NtcFilesTest.java (1)
9-9: LGTM!The added import for
CseDataExceptionis necessary for the new test cases.cse-lib/data/src/test/resources/com/farao_community/farao/cse/data/ntc/20210624_2D4_NTC_reductions_CSE1_missingPeriodForSpecialLine.xml (4)
1-12: LGTM! Document metadata is well-structured and complete.The metadata section includes all necessary fields with appropriate values, and the schema reference ensures proper validation.
497-509: Verify handling of missing time intervals in special line definition.The special line "mendrisio-cagno" has:
- Only one time interval (00:00-01:00) defined
- A long-term period spanning multiple years
- Matching NTC and fixed flow values (-80)
This appears to be intentionally configured for testing missing period scenarios, but verify that the code handles undefined time intervals appropriately.
Run this script to check special line handling:
✅ Verification successful
Missing time intervals are properly handled by the codebase
The file is intentionally structured with a single time interval for testing purposes. The codebase includes:
- Robust handling of missing/undefined time intervals through Optional returns
- Specific test cases verifying behavior with missing periods
- Proper fallback mechanisms when intervals are not found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for special line handling in code # Test: Look for special line period handling logic ast-grep --pattern 'class $_ { $$$ getSpecialLine($_, $_) { $$$ } $$$ }'Length of output: 86
Script:
#!/bin/bash # Search for special line related code rg -l "SpecialLine" --type java # Search for time interval handling rg "TimeInterval|DayOfWeek|Period" --type java -A 3 # Look for XML parsing of special lines ast-grep --pattern 'class $_ implements $_Parser { $$$ parse($_) { $$$ } $$$ }'Length of output: 39694
13-254: Verify the NTC value transitions and country totals.The NTC values show two distinct sets across the time periods:
- Night hours (22:00-05:00): Total NTC = 5400 MW
- Day hours (05:00-22:00): Total NTC = 6005 MW
The transitions between these periods and the country-wise distribution appear consistent.
Run this script to validate the NTC values and transitions:
✅ Verification successful
NTC transitions verified and consistent
The NTC values show proper transitions with:
- Clear day/night pattern (5400 MW total at night, 6005 MW during day)
- Consistent country proportions maintained
- Correct transition timing at 05:00Z and 21:00Z
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Validate NTC values and transitions # Test: Check if NTC values sum up correctly and transitions occur at expected times rg -A 4 '<TimeInterval.*T05:00Z' | rg '<NTC.*v="' rg -A 4 '<TimeInterval.*T22:00Z' | rg '<NTC.*v="'Length of output: 73659
Script:
#!/bin/bash # Focus search on specific file and transition periods echo "=== 05:00Z Transition ===" rg -A 4 '<TimeInterval.*T05:00Z' cse-lib/data/src/test/resources/com/farao_community/farao/cse/data/ntc/20210624_2D4_NTC_reductions_CSE1_missingPeriodForSpecialLine.xml echo -e "\n=== 22:00Z Transition ===" rg -A 4 '<TimeInterval.*T22:00Z' cse-lib/data/src/test/resources/com/farao_community/farao/cse/data/ntc/20210624_2D4_NTC_reductions_CSE1_missingPeriodForSpecialLine.xmlLength of output: 2759
255-496: Verify splitting factors sum to 1.0 and align with NTC transitions.The splitting factors show appropriate distribution:
- Night hours (22:00-05:00): Sum ≈ 1.0 (0.480545 + 0.389105 + 0.037938 + 0.092412)
- Day hours (05:00-22:00): Sum ≈ 1.0 (0.466321 + 0.409326 + 0.035406 + 0.088946)
The transitions align with NTC value changes, maintaining consistency.
Run this script to validate the splitting factors:
✅ Verification successful
Splitting factors validation successful
All splitting factors sum to 1.0 (within acceptable decimal precision of 0.0001) across all time periods. The transitions between different factor sets align correctly with the NTC value changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Validate splitting factors sum to 1.0 # Test: Sum up splitting factors for both time ranges rg -A 4 '<TimeInterval.*T05:00Z' | rg '<SplittingFactor.*v="' rg -A 4 '<TimeInterval.*T22:00Z' | rg '<SplittingFactor.*v="'Length of output: 73765
Script:
#!/bin/bash # Create Python script to validate splitting factors cat << 'EOF' > validate_factors.py import xml.etree.ElementTree as ET import sys from decimal import Decimal def validate_splitting_factors(xml_file): tree = ET.parse(xml_file) root = tree.getroot() for period in root.findall('.//Period'): dtini = period.get('dtini') for interval in period.findall('.//TimeInterval'): factors = [] for factor in interval.findall('SplittingFactor'): factors.append(Decimal(factor.get('v'))) total = sum(factors) if abs(total - 1) > Decimal('0.0001'): print(f"Period {dtini}: Sum = {total}") for xml_file in sys.argv[1:]: validate_splitting_factors(xml_file) EOF # Find and validate all relevant XML files find . -name "*NTC_reductions_CSE1*.xml" -exec python validate_factors.py {} \;Length of output: 7468
Script:
#!/bin/bash # Create improved Python script to validate splitting factors cat << 'EOF' > validate_factors.py import xml.etree.ElementTree as ET import sys from decimal import Decimal def validate_splitting_factors(xml_file): tree = ET.parse(xml_file) root = tree.getroot() # Debug XML structure print(f"\nProcessing {xml_file}") for period in root.findall('.//SplittingFactors/Period'): dtini = period.get('dtini') for dow in period.findall('DayOfWeek'): for interval in dow.findall('TimeInterval'): tini = interval.get('tini') factors = [] for factor in interval.findall('SplittingFactor'): v = Decimal(factor.get('v')) country = factor.get('country') factors.append((country, v)) total = sum(v for _, v in factors) if abs(total - 1) > Decimal('0.0001'): print(f"Period {dtini} {tini}: Sum = {total}") for country, v in factors: print(f" {country}: {v}") # Process only the file under review xml_file = "cse-lib/data/src/test/resources/com/farao_community/farao/cse/data/ntc/20210624_2D4_NTC_reductions_CSE1_missingPeriodForSpecialLine.xml" validate_splitting_factors(xml_file) EOF python validate_factors.pyLength of output: 182
Script:
#!/bin/bash # First find the files and store the path echo "Locating XML files..." find . -name "*NTC_reductions_CSE1*.xml" -type f # Create improved Python script with error handling cat << 'EOF' > validate_factors.py import xml.etree.ElementTree as ET import sys from decimal import Decimal import os def validate_splitting_factors(xml_file): try: if not os.path.exists(xml_file): print(f"File not found: {xml_file}") return print(f"\nValidating {xml_file}") tree = ET.parse(xml_file) root = tree.getroot() # Print first few levels of XML structure for debugging print("XML structure:") for child in root[:2]: print(f"- {child.tag}") for subchild in child[:2]: print(f" - {subchild.tag}") for period in root.findall(".//Period"): dtini = period.get('dtini') if dtini and 'SplittingFactor' in period.find('.//TimeInterval/SplittingFactor').tag: for interval in period.findall('.//TimeInterval'): tini = interval.get('tini') factors = [] for factor in interval.findall('SplittingFactor'): v = Decimal(factor.get('v')) country = factor.get('country') factors.append((country, v)) total = sum(v for _, v in factors) if abs(total - 1) > Decimal('0.0001'): print(f"Period {dtini} {tini}: Sum = {total}") for country, v in factors: print(f" {country}: {v}") else: print(f"Period {dtini} {tini}: Valid (sum = {total})") except Exception as e: print(f"Error processing {xml_file}: {str(e)}") # Process each file found for xml_file in sys.argv[1:]: validate_splitting_factors(xml_file) EOF # Find and process each XML file find . -name "*NTC_reductions_CSE1*.xml" -type f -exec python validate_factors.py {} \;Length of output: 3796
Script:
#!/bin/bash # Create improved Python script with namespace handling cat << 'EOF' > validate_factors.py import xml.etree.ElementTree as ET import sys from decimal import Decimal import os def validate_splitting_factors(xml_file): try: if not os.path.exists(xml_file): print(f"File not found: {xml_file}") return print(f"\nValidating {xml_file}") tree = ET.parse(xml_file) root = tree.getroot() # Debug full XML structure print("XML structure:") def print_structure(elem, level=0): print(" " * level + f"- {elem.tag}") if level < 3: # Only show first 3 levels for child in elem[:2]: # Show first 2 children at each level print_structure(child, level + 1) print_structure(root) # Find SplittingFactors section splitting_factors = root.find('.//SplittingFactors') if splitting_factors is None: print("No SplittingFactors section found") return print("\nProcessing splitting factors...") for period in splitting_factors.findall('Period'): dtini = period.get('dtini') for dow in period.findall('DayOfWeek'): for interval in dow.findall('TimeInterval'): tini = interval.get('tini') factors = [] for factor in interval.findall('SplittingFactor'): v = Decimal(factor.get('v')) country = factor.get('country') factors.append((country, v)) if factors: total = sum(v for _, v in factors) if abs(total - 1) > Decimal('0.0001'): print(f"Period {dtini} {tini}: Sum = {total}") for country, v in factors: print(f" {country}: {v}") else: print(f"Period {dtini} {tini}: Valid (sum = {total})") except Exception as e: print(f"Error processing {xml_file}: {str(e)}") # Process the file under review xml_file = "./cse-lib/data/src/test/resources/com/farao_community/farao/cse/data/ntc/20210624_2D4_NTC_reductions_CSE1_missingPeriodForSpecialLine.xml" validate_splitting_factors(xml_file) EOF python validate_factors.pyLength of output: 1673
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cse-lib/data/src/test/java/com/farao_community/farao/cse/data/ntc/NtcFilesTest.java (2)
82-94: Consider renaming test method for consistency.The test method name could be more consistent with existing test method names in the class. For example,
checkFixedFlowWithMissingPeriodInNtcRedFilewould align better withcheckDefaultFlowForMendrisioCagno.
82-110: Reduce code duplication and improve test coverage.
Consider extracting common setup code into helper methods to reduce duplication:
- Resource loading
- NTC object creation
- Exception handling
Consider adding test cases for edge scenarios:
- Missing periods in both annual and daily files
- Invalid data in the annual file when used as backup
Example refactoring:
private Ntc createNtc(String yearlyDataFile, String dailyDataFile, OffsetDateTime targetDateTime) { try (InputStream yearlyData = getClass().getResourceAsStream(yearlyDataFile); InputStream dailyData = getClass().getResourceAsStream(dailyDataFile)) { return new Ntc( new YearlyNtcDocument(targetDateTime, DataUtil.unmarshalFromInputStream(yearlyData, NTCAnnualDocument.class)), new DailyNtcDocument(targetDateTime, DataUtil.unmarshalFromInputStream(dailyData, NTCReductionsDocument.class)), false ); } catch (IOException | JAXBException e) { throw new CseInvalidDataException("Impossible to create NTC", e); } }Then the test methods could be simplified:
@Test void checkFixedFlowWithMissingPeriodInNtcRedFile() { OffsetDateTime targetDateTime = OffsetDateTime.parse("2021-06-24T16:30Z"); Ntc ntc = createNtc( "2021_2Dp_NTC_annual_CSE1.xml", "20210624_2D4_NTC_reductions_CSE1_missingPeriodForSpecialLine.xml", targetDateTime ); Map<String, Double> fixedFlowLines = ntc.getFlowOnFixedFlowLines(); assertEquals(150, fixedFlowLines.get(MENDRISIO_CAGNO_ID), DOUBLE_PRECISION); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocument.java(3 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocumentAdapted.java(3 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtil.java(1 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtilAdapted.java(1 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocument.java(1 hunks)cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocumentAdapted.java(1 hunks)cse-lib/data/src/test/java/com/farao_community/farao/cse/data/ntc/NtcFilesTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocument.java
- cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/YearlyNtcDocumentAdapted.java
- cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtil.java
- cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/NtcUtilAdapted.java
- cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocumentAdapted.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
cse-lib/data/src/main/java/com/farao_community/farao/cse/data/ntc/DailyNtcDocument.java (1)
18-18: LGTM!The Optional import is correctly placed and necessary for the new changes.
cse-lib/data/src/test/java/com/farao_community/farao/cse/data/ntc/NtcFilesTest.java (2)
9-9: LGTM!The addition of
CseDataExceptionimport is appropriate for the new test case that verifies exception handling.
96-110: LGTM!The error handling test is well-structured and verifies the expected exception message.
| @@ -27,7 +28,7 @@ public DailyNtcDocument(OffsetDateTime targetDateTime, NTCReductionsDocument ntc | |||
| this.ntcReductionsDocument = ntcReductionsDocument; | |||
| } | |||
|
|
|||
| Map<String, LineInformation> getLineInformationPerLineId(Predicate<TLine> lineSelector) { | |||
| Map<String, Optional<LineInformation>> getLineInformationPerLineId(Predicate<TLine> lineSelector) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the design by avoiding Optional wrapping.
As previously suggested by @vbochetRTE, wrapping every value in Optional might be overcomplicating the design. Instead, consider only including lines that have actual data in the map.
Here's a simpler approach:
- Map<String, Optional<LineInformation>> getLineInformationPerLineId(Predicate<TLine> lineSelector) {
+ Map<String, LineInformation> getLineInformationPerLineId(Predicate<TLine> lineSelector) {
List<TSpecialLines> tSpecialLines = ntcReductionsDocument.getSpecialLines();
if (tSpecialLines.isEmpty()) {
return Collections.emptyMap();
}
if (tSpecialLines.size() == 1) {
return tSpecialLines.get(0).getLine().stream()
.filter(lineSelector)
+ .filter(tLine -> NtcUtil.getTNtcFromLine(targetDateTime, tLine).isPresent())
.collect(Collectors.toMap(
TLine::getCode,
- tLine -> {
- Optional<TNTC> optionalTntc = NtcUtil.getTNtcFromLine(targetDateTime, tLine);
- if (optionalTntc.isPresent()) {
- return Optional.of(new LineInformation(tLine.getCNtc().value(), optionalTntc.get().getType(), optionalTntc.get().getV().doubleValue()));
- } else {
- return Optional.empty();
- }
- }
+ tLine -> {
+ TNTC tntc = NtcUtil.getTNtcFromLine(targetDateTime, tLine).get();
+ return new LineInformation(tLine.getCNtc().value(), tntc.getType(), tntc.getV().doubleValue());
+ }
));
}
throw new CseDataException("Several special lines sections have been defined");
}Benefits of this approach:
- Simpler and more maintainable code
- Better performance by avoiding Optional overhead
- Clearer contract: if a line is in the map, it has valid data
- Callers don't need to handle Optional unwrapping
Also applies to: 42-47
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
Improvements
New Features
Technical Updates
Optionaltype.