Closed
Conversation
Review Summary by QodoRefactor MSW export to use flat RigMswSegment list with multiple code paths
WalkthroughsDescription• Introduce RigMswSegment as primary building block for flat MSW export - Replaces tree-based segment representation with flat list approach - Embeds WELSEGS fields, cell intersections, and valve data in single struct • Add three alternative code paths for MSW segment generation - Legacy path: existing tree-based collectWelsegsData (deprecated) - Direct tree path: buildFlatSegmentsDirect traverses tree inline - Geometry path: buildMswFromGeometry builds directly from well-path geometry • Implement collectDataFromFlatList() to populate table data from flat segments - Replaces recursive tree traversal with simple iteration - Deduplicates COMPSEGS entries across segments • Add comprehensive geometry-based builders for all completion types - Main-bore segments, valve (ICD/AICD/SICD/ICV), fracture, and fishbones segments - Support for tie-in laterals with recursive child well processing Diagramflowchart LR
A["RimWellPath<br/>Geometry"] -->|buildMswFromGeometry| B["RigMswSegment<br/>List"]
C["RicMswBranch<br/>Tree"] -->|buildFlatSegmentsDirect| B
C -->|Legacy Path| D["RigMswTableData<br/>Tables"]
B -->|collectDataFromFlatList| D
B -->|buildFlatMswSegments| E["RigMswFlatExportData<br/>Header + Segments"]
E -->|extractSingleWellMswData| D
File Changes1. ApplicationLibCode/ReservoirDataModel/CompletionsMsw/RigMswSegment.h
|
Code Review by Qodo
1. COMPSEGS dedup removed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+48
to
+80
| std::set<size_t> intersectedCells; // deduplicate COMPSEGS entries across segments | ||
|
|
||
| for ( const auto& seg : exportData.segments ) | ||
| { | ||
| // WELSEGS row | ||
| WelsegsRow welsegsRow; | ||
| welsegsRow.segment1 = seg.segmentNumber; | ||
| welsegsRow.segment2 = seg.segmentNumber; | ||
| welsegsRow.branch = seg.branchNumber; | ||
| welsegsRow.joinSegment = seg.outletSegmentNumber; | ||
| welsegsRow.length = seg.length; | ||
| welsegsRow.depth = seg.depth; | ||
| welsegsRow.diameter = seg.diameter; | ||
| welsegsRow.roughness = seg.roughness; | ||
| welsegsRow.description = seg.description; | ||
| welsegsRow.sourceWellName = seg.sourceWellName; | ||
| tableData.addWelsegsRow( welsegsRow ); | ||
|
|
||
| // COMPSEGS / COMPSEGL rows — deduplicated across segments | ||
| for ( const auto& inter : seg.intersections ) | ||
| { | ||
| // Use a simple hash for deduplication: combine i, j, k, gridName | ||
| // Reuse the intersectedCells set via a packed index trick; for LGR cells we skip dedup | ||
| CompsegsRow compRow; | ||
| compRow.i = inter.i; | ||
| compRow.j = inter.j; | ||
| compRow.k = inter.k; | ||
| compRow.branch = seg.branchNumber; | ||
| compRow.distanceStart = inter.distanceStart; | ||
| compRow.distanceEnd = inter.distanceEnd; | ||
| compRow.gridName = inter.gridName; | ||
| tableData.addCompsegsRow( compRow ); | ||
| } |
There was a problem hiding this comment.
1. Compsegs dedup removed 🐞 Bug ✓ Correctness
RicMswTableDataTools::collectDataFromFlatList() declares intersectedCells but never uses it, so duplicate COMPSEGS/COMPSEGL rows can be emitted for the same cell across segments/completions. The legacy collector deduplicated by globalCellIndex, so this change can produce invalid/duplicated export rows.
Agent Prompt
## Issue description
`RicMswTableDataTools::collectDataFromFlatList()` emits one COMPSEGS/COMPSEGL row per `RigMswCellIntersection` without deduplication, even though it declares `intersectedCells`. This regresses behavior vs the legacy collectors (`collectCompsegDataByType`) which deduplicate by cell to avoid duplicated COMPSEGS rows.
## Issue Context
In the legacy path, `intersectedCells` is checked/updated using `globalCellIndex` before adding a COMPSEGS row. In the new flat-list path, we only have (i,j,k,gridName) per intersection, so dedup needs to be reintroduced using an equivalent key.
## Fix Focus Areas
- ApplicationLibCode/Commands/CompletionExportCommands/RicMswTableDataTools.cpp[44-94]
- ApplicationLibCode/Commands/CompletionExportCommands/RicMswTableDataTools.cpp[650-747]
## Implementation notes
- Implement a dedup key such as `(gridName, i, j, k)` (and if needed `distanceStart/distanceEnd` depending on intended semantics).
- Use `intersectedCells` (or rename it) as an `unordered_set`/`set` of that key.
- Ensure main-grid and LGR intersections are deduped consistently with how `RigMswTableData::mainGridCompsegsData()`/`lgrCompsegsData()` split rows (likely based on `gridName`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+491
to
+505
| if ( splitSegs.size() > 1 ) | ||
| { | ||
| ( *segmentNumber )++; | ||
| treeSegment->setSegmentNumber( *segmentNumber ); | ||
| } | ||
|
|
||
| treeSegment->setOutputMD( midPointMD ); | ||
| treeSegment->setOutputTVD( midPointTVD ); | ||
| treeSegment->setSegmentNumber( *segmentNumber ); | ||
|
|
||
| curPrevOutMD = midPointMD; | ||
| curPrevOutTVD = midPointTVD; | ||
| } | ||
|
|
||
| if ( splitSegs.size() <= 1 ) ( *segmentNumber )++; |
There was a problem hiding this comment.
2. Split segment off-by-one 🐞 Bug ✓ Correctness
buildFlatSegmentsRecursive() increments *segmentNumber and assigns treeSegment->segmentNumber() during sub-segmentation in a way that leaves the RicMswSegment with a different segmentNumber than the last emitted RigMswSegment. Later code uses treeSegment->segmentNumber() as the outlet for completions/child branches, so outlets can reference a segment number that was never emitted as a WELSEGS row.
Agent Prompt
## Issue description
In `buildFlatSegmentsRecursive()`, when a tree segment is split into multiple subsegments, the logic increments `*segmentNumber` and assigns `treeSegment->setSegmentNumber(*segmentNumber)` in-loop. This causes `treeSegment->segmentNumber()` to drift to a number that does not correspond to the last emitted WELSEGS segment, but later code uses `treeSegment->segmentNumber()` as an outlet for completions and child branches.
## Issue Context
The flat builder emits `RigMswSegment seg` using `seg.segmentNumber = *segmentNumber`, then mutates the `treeSegment` and increments `*segmentNumber`. `processCompletionsForSegment()` and child-branch recursion subsequently consult `treeSegment->segmentNumber()` / `outletForChild->segmentNumber()`.
## Fix Focus Areas
- ApplicationLibCode/Commands/CompletionExportCommands/RicWellPathExportMswTableData.cpp[409-546]
- ApplicationLibCode/Commands/CompletionExportCommands/RicWellPathExportMswTableData.cpp[280-325]
## Implementation notes
- Make the segment numbering deterministic by using a local `int currentSegNum = *segmentNumber;` and emitting subsegments as `subSegNum = currentSegNum++`.
- After emitting all subsegments for the tree segment, set `*segmentNumber = currentSegNum`.
- Ensure `treeSegment->setSegmentNumber(lastEmittedSubSegNum)` (not `*segmentNumber`) so outlets for completions/child branches point to an actually-emitted WELSEGS segment.
- Add a small assertion or comment to document the invariant: `treeSegment->segmentNumber()` must match a segmentNumber present in `result`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+563
to
+571
| WsegvalvRow row; | ||
| row.well = wellName; | ||
| row.segmentNumber = segNum; | ||
| row.cv = valve->flowCoefficient(); | ||
| row.area = valve->area( branch->wellPath()->unitSystem() ); | ||
| row.status = "OPEN"; | ||
| row.description = valve->name().toStdString(); | ||
| result[row.segmentNumber] = row; | ||
| } |
There was a problem hiding this comment.
3. Wsegvalv rows overwritten 🐞 Bug ✓ Correctness
buildFlatSegmentsDirect() collects WSEGVALV data into a std::map keyed by segmentNumber, so multiple valves that map to the same segment overwrite each other and are lost. The legacy path appended one WSEGVALV row per valve, and the data model (RimValveCollection) supports multiple active valves.
Agent Prompt
## Issue description
`buildFlatSegmentsDirect()` aggregates WSEGVALV into `std::map<int, WsegvalvRow>`, overwriting entries when multiple valves map to the same segment number. This silently drops valve rows compared to the legacy behavior which appended all valve rows.
## Issue Context
- `RimValveCollection::activeValves()` returns a vector of potentially many valves.
- Legacy export appended one `WsegvalvRow` per valve (`addWsegvalvRow`).
- New path uses `result[row.segmentNumber] = row;` (overwrite).
## Fix Focus Areas
- ApplicationLibCode/Commands/CompletionExportCommands/RicWellPathExportMswTableData.cpp[548-635]
- ApplicationLibCode/Commands/CompletionExportCommands/RicWellPathExportMswTableData.cpp[3442-3453]
- ApplicationLibCode/Commands/CompletionExportCommands/RicMswTableDataTools.cpp[794-822]
## Implementation notes
Choose one (consistent) representation:
1) **Preferred:** change flat model to support multiple valve rows per segment:
- Change `RigMswSegment::wsegvalvData` from `optional<WsegvalvRow>` to `vector<WsegvalvRow>` (and update `collectDataFromFlatList()` to add all rows).
- Update `collectStandaloneWsegvalvBySegmentRecursive` / `collectWsegvalvBySegmentRecursive` to `map<int, vector<WsegvalvRow>>` or directly append to a `vector<WsegvalvRow>`.
2) If the export format truly requires one valve per segment, enforce/validate that invariant explicitly and emit a warning/error when collisions occur (do not overwrite silently).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…appingTools test Guard NNC cell indices against out-of-bounds in addNnc. Fix test using ActiveCellIndex(1) on a single-element vector.
…unit tests - Make findFishboneLateralsWellBoreParts public and return result by value - Move WellBorePartForTransCalc to its own header RicWellBorePartForTransCalc.h - Replace MSW tree traversal with direct RimFishbones lateral geometry iteration - Add setSubsOrientationMode() to RimFishbones for deterministic test angles - Add unit tests validating intersected cells and result values
…able Each RigMswSegment holds all fields needed for one WELSEGS row, plus cell intersections (COMPSEGS/COMPSEGL) and optional valve data (WSEGVALV/WSEGAICD/WSEGSICD).
Adds addSegment()/segments() alongside the existing mainBoreBranch tree to allow incremental migration to the new flat segment approach.
Adds buildFlatMswSegmentList() that converts WelsegsRows to RigMswSegment objects, joins cell intersections from the tree (by segment number) and valve data from the WSEGVALV/WSEGAICD/WSEGSICD rows. The list is stored on RigMswTableData and returned to callers alongside the existing tables.
buildFlatMswSegments() builds the tree internally, runs all collection functions, and returns a flat RigMswFlatExportData (header + segment list) without exposing the tree to callers. collectDataFromFlatList() in RicMswTableDataTools iterates the flat list to populate RigMswTableData, replacing all recursive tree traversals. extractSingleWellMswData() is refactored to call buildFlatMswSegments() + collectDataFromFlatList(), reducing it to ~5 lines.
…acy collectWelsegsData path Add buildFlatSegmentsDirect() that traverses the RicMswBranch tree directly and produces RigMswSegment objects without going through collectWelsegsData/collectCompsegData or the intermediate WelsegsRow/RigMswTableData structures. Both code paths coexist with a constexpr useLegacyPath switch in buildFlatMswSegments() for A/B comparison.
Add [[deprecated]] attributes to the top-level entry points of the legacy RicMswBranch tree traversal pipeline: collectWelsegsData, collectWelsegsDataRecursively, collectCompsegData, collectCompsegDataByType, collectWsegvalvData, collectStandaloneWsegvalvDataRecursively, collectWsegvalvDataRecursively, collectWsegAicdData, collectWsegSicdData, and buildFlatMswSegmentList. All functions are kept; callers should migrate to buildFlatMswSegments().
Add two helpers to the internal namespace: - collectStandaloneWsegvalvBySegmentRecursive: collects WSEGVALV rows from the well's standalone valve collection (not from completions), matching each valve to the nearest branch segment by MD. - collectWsegvalvBySegmentRecursive: collects WSEGVALV rows from ICD/ICV completion-based valves and tie-in ICVs, keyed by segment number. buildFlatSegmentsDirect joins both maps into the flat segment list, matching by segment number. This fills the last gap vs. the legacy path (AICD and SICD were already handled). The direct path now covers all three valve table types: WSEGVALV, WSEGAICD, WSEGSICD.
Set useLegacyPath = false so buildFlatMswSegments now uses buildFlatSegmentsDirect exclusively. The direct path traverses the RicMswBranch tree once and writes RigMswSegment objects inline, covering all three valve table types (WSEGVALV, WSEGAICD, WSEGSICD). The legacy collectWelsegsData / buildFlatMswSegmentList code path is no longer exercised but is retained until output equivalence is confirmed.
…rforation COMPSEGS) First building block for building MSW flat segments without using any RicMswItem tree. - internal::toMswCellIntersection(): converts WellPathCellIntersectionInfo globCellIndex to RigMswCellIntersection (1-based i,j,k + grid name) using the main grid - internal::buildMainBoreSegmentsFromGeometry(): creates main-bore RigMswSegment objects directly from filtered cell intersections; embeds COMPSEGS entries for cells that overlap active perforation intervals (no valve completions, fishbones, or fractures yet) - RicWellPathExportMswTableData::buildMswFromGeometry(): entry point; replaces the full generateWellSegmentsForMswExportInfo + buildFlatSegmentsDirect pipeline with a call to buildMainBoreSegmentsFromGeometry; marked with TODO comments for missing pieces The existing buildFlatSegmentsDirect path remains the production default; this new path is the first step toward removing all RicMswItem usage from the export pipeline. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
buildMswFromGeometry now builds valve (ICD/ICV/AICD/SICD) WELSEGS segments directly from RimPerforationInterval/RimWellPathValve without any RicMswItem types. Main bore COMPSEGS are suppressed for perforation intervals that have an active valve; the valve segment carries those COMPSEGS instead. WSEGVALV data (Cv, area) is populated for ICD and ICV valves. WSEGAICD/WSEGSICD accumulation is left as a TODO for the next commit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
buildMswFromGeometry now builds fracture WELSEGS segments directly from RimWellPathFracture without any RicMswItem types. Each fracture produces one WELSEGS segment branching off the nearest main-bore outlet segment. COMPSEGS entries are populated from RicExportFractureCompletionsImpl and dual porosity K-shifts are applied. ALONG_WELL_PATH fractures use the perforation-length range rather than the raw fracture width. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
buildMswFromGeometry now builds fishbones ICD/lateral WELSEGS segments directly from RimFishbones without any RicMswItem types. For each sub location an ICD segment is created with COMPSEGS covering the cells closest to the sub and a WSEGVALV entry (Cv, area). Each lateral gets its own branch with one WELSEGS sub-segment per cell intersection along the lateral's geometry (computed via RigWellPathIntersectionTools). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
buildMswFromGeometry now handles tie-in child well paths recursively via
the new private buildLateralSegments() method. For each child:
- An optional ICV at the tie-in point is emitted as a WELSEGS segment
with WSEGVALV data, branching off the parent main-bore segment.
- The child's own main-bore, valve, fracture, and fishbones segments
are built with the same direct-geometry helpers as the parent.
- Grandchildren are handled by recursion.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
buildFlatMswSegments now has a useGeometryPath switch (alongside the existing useLegacyPath) to opt into buildMswFromGeometry, which builds segments directly from well-path geometry without any RicMswItem tree. Both flags default to false; flip useGeometryPath to true to test the new path end-to-end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
magnesj
force-pushed
the
msw-export-simplified
branch
from
7fc4d7d to
b4650cf
Compare
…afPdmObjectGroup Remove the unused non-template PdmObjectCollection class from cafPdmObjectGroup.h/.cpp. This class conflicted with the template caf::PdmObjectCollection<T> in cafPdmObjectCollection.h, causing MSVC redefinition errors in Unity builds. The class was dead code with no usages in application code. Also correct forward declarations in test MainWindow headers from PdmObjectCollection to PdmObjectGroup, matching actual usage in the .cpp files.
Extract the direct geometry path implementation from RicWellPathExportMswTableData.cpp into a new RicWellPathExportMswGeometryPath.cpp file. This separates the two alternative code paths (tree-based and direct geometry) into distinct compilation units. Move valveSegmentLength constant to RicMswTableDataTools namespace in the shared header to avoid duplication and Unity build conflicts.
…tions Move buildMswFromGeometry and buildLateralSegments out of RicWellPathExportMswTableData into a dedicated RicWellPathExportMswGeometryPath namespace, declared in the new RicWellPathExportMswGeometryPath.h header. Move the four shared geometry helpers (generateCellSegments, computeIntitialMeasuredDepth, filterIntersections, wellPathsWithTieIn) from private to public in RicWellPathExportMswTableData so they can be called by the free functions in the geometry path namespace.
Remove buildFlatSegmentsDirect and its three exclusive internal helpers (buildFlatSegmentsRecursive, collectStandaloneWsegvalvBySegmentRecursive, collectWsegvalvBySegmentRecursive). These were dead code since buildFlatMswSegments always routed to the direct geometry path. Simplify buildFlatMswSegments to directly call buildMswFromGeometry. Remove the unused exportCompletionsAfterMainBoreSegments parameter from extractSingleWellMswData and buildFlatMswSegments, updating all call sites.
No callers remain after the refactoring to buildFlatMswSegments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…put param Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…GeometryPath Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.