Repository navigation
fix(Geosciences): robustness and performance review - #205
Conversation
…te declaration Co-Authored-By: Claude Opus 5.5 <[email protected]>
Corrupted files could request a huge allocation when reading the CRS authority, code or name. Use MAX_SERIALIZED_NAME_SIZE as for other serialized names, the wire format is unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…_coordinates SetFromUserInput errors were ignored and a null transformation was dereferenced, crashing on unknown or empty authority codes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… failure The OGR transformation leaked when a point conversion threw. Own it with a unique_ptr using OGRCoordinateTransformation::DestroyCT. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Without OAMS_TRADITIONAL_GIS_ORDER, GDAL uses the authority axis order and geographic CRSs such as EPSG:4326 expect latitude first, swapping the coordinates of meshes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…iled mesh conversion The new CRS attribute was created before checking that the active CRS is geographic, and kept when the coordinate import failed. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ning info The attribute was renamed before registering the new CRS, which threw when the name already existed, including when reusing the active CRS name. Check the name first and support replacing the active CRS under the same name. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ations atomic Converting or assigning a CRS on a model mutated components one by one, so an invalid component (existing CRS name, non geographic or unknown active CRS, failed point conversion) left the model half converted. Validate all components and compute all converted coordinates before modifying any mesh. The coordinate transformation is factored in an internal GeographicCoordinateTransformation class. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ons across model components BRep and Section conversions created two OGRSpatialReference and one coordinate transformation per component. Create them once per source CRS. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Call OGRCoordinateTransformation::Transform once on coordinate arrays instead of once per point, and report the number of points that failed to convert. Conversion still throws if any point fails. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…uctors StructuralModel and CrossSection constructors taking an initial model, a BRep/Section and a component mapping call transfer_geological_information, which throws when the mapping refers to components missing from the new model. Being noexcept, they called std::terminate instead of propagating the exception. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Horizon, Fault, FaultBlock and StratigraphicUnit item ranges of StructuralModel and CrossSection dereferenced every item of the collection as a Surface/Block (resp. Line/Surface), throwing as soon as an item of another type was in the collection (e.g. added through the public add_item_in_collection or read from a corrupted file). Items with an unexpected component type are now skipped when the range is created and when it is incremented. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ransfer When several initial components map to the same new component, transfer_geological_information added the same item to the same geological collection repeatedly, each time scanning the relationships and logging a "relation already exists" warning. Items already added to a collection during the transfer are now skipped. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Fault type and Horizon contact type are deserialized as raw integers and were never validated, so a corrupted file could yield enum values outside of FAULT_TYPE / CONTACT_TYPE. Such values are now reset to no_type (Fault) or conformal (Horizon) with a warning. The legacy Horizon contact type conversion also warns instead of silently falling back to conformal on unknown values. Co-Authored-By: Claude Opus 5.5 <[email protected]>
add_geology_clone_mapping emplaced the Fault and FaultBlock mappings, throwing when the given ModelCopyMapping already had an entry for these types, whereas Horizon and StratigraphicUnit mappings were merged. Faults and FaultBlocks are now merged the same way, keeping existing entries. Also drop the meaningless [[nodiscard]] on the void add_horizons_clone_mapping and add_stratigraphic_units_clone_mapping helpers (no behavior change, hence not separately tested). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…nt mappings add_geological_components_mappings emplaced one GenericMapping per geological component type, throwing when the ModelGenericMapping already had an entry for that type. Geological mappings are now merged into the existing entries, and missing geological types are ignored instead of throwing. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ctivity Faults, Horizons, FaultBlocks and StratigraphicUnits can now be set active or inactive through set_fault_active, set_horizon_active, set_fault_block_active and set_stratigraphic_unit_active, mirroring the OpenGeode mesh and collection component builders. The setters are also exposed in the Python bindings. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Copying or cloning a StructuralModel or CrossSection created the geological components as active regardless of the source ones. The activity of Faults, Horizons, FaultBlocks and StratigraphicUnits is now copied, as OpenGeode does for mesh and collection components. Co-Authored-By: Claude Opus 5.5 <[email protected]>
StructuralModelBuilder::copy and CrossSectionBuilder::copy did not check that the target model was empty, unlike BRepBuilder::copy and SectionBuilder::copy, silently mixing copied components with existing ones. They now throw when the target has any component, geological components included, pointing to ModelConcatener. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…s flood fill build_structural_model_fault_blocks tracked visited blocks in a vector searched linearly (quadratic in the number of blocks) and, for each block boundary, scanned the items of every fault to know whether the surface was a fault. Visited blocks and fault surfaces are now stored in hash sets, the latter computed once. The unused treated_faults vector is removed. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ult block Calling build_structural_model_fault_blocks on a model which already has FaultBlocks (e.g. calling it twice) created new FaultBlocks for every Block, so Blocks ended up in several FaultBlocks. Blocks already belonging to a FaultBlock are now kept as is and ignored when building the new FaultBlocks, with a warning, making the function idempotent. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… bound stack walks add_above_relation now throws when the new relation would close a cycle (or relate a component to itself). is_above, compute_top_and_bottom_horizons, the ordered ranges and repair_horizon_stack_if_possible bound their walks so that inconsistent relations can no longer cause infinite loops. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Corrupted files containing a cycle in above relations are repaired on load: the relations closing a cycle are removed with a warning. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ve_relation Adding "A above B" when "B above A" already existed silently kept the old direction. The existing relation is now reversed (with a warning), as long as it does not create a cycle. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…e/under set_horizon_above and set_horizon_under now require the horizon and the stratigraphic unit to belong to the stack, and replace (with a warning) the previous component directly above/under instead of adding duplicated relations that made above()/under() ambiguous. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ition add_horizon_in_stratigraphic_unit, remove_horizon and remove_stratigraphic_unit now recompute the top and bottom horizons when they were already computed, instead of leaving stale (possibly removed) horizons. compute_top_and_bottom_horizons resets them on an empty stack. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… stacks The ordered ranges and string() dereferenced above()/under()/top_horizon() optionals without checking them, throwing bad_optional_access on stacks with missing units or gaps. Iteration now ends where the relations stop and string() skips missing units. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…mes helpers horizons_stack_from_bottom_to_top_names and horizons_stack_from_top_to_bottom_names accessed units_names[0] out of bounds when both lists were empty. An empty stack is now returned. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tack_if_possible Repairing a stack with no horizon threw bad_optional_access on the missing bottom horizon; there is nothing to repair in that case. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Replace the private relation_edge scan (comparing component uuids around a vertex) by the base relation_edge_index, which compares vertex indices around the lowest degree vertex. remove_above_relation no longer performs a redundant second lookup since the lookup is undirected. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… initial model and BRep The ImplicitStructuralModel( initial, BRep&&, mappings ) and ImplicitCrossSection( initial, Section&&, mappings ) constructors always created a new implicit attribute with a random id, so implicit values carried by the new meshes under the initial model implicit attribute (e.g. remeshed copies) were lost and a duplicated geode_implicit_attribute was left. When the new meshes carry the initial implicit attribute, its id is now adopted; otherwise the behavior is unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ng in noexcept constructors Converting constructors of StratigraphicModel and StratigraphicSection were noexcept but threw when a Block (resp. Surface) was not meshed as a TetrahedralSolid (resp. TriangulatedSurface), calling std::terminate. Such components are now skipped, consistently with ImplicitStructuralModel, the converting constructors are no longer noexcept and the move constructors no longer re-initialize the stratigraphic queries. Co-Authored-By: Claude Opus 5.5 <[email protected]>
stratigraphic_containing_polyhedron (resp. polygon) evaluated the distance to the element returned by the AABB tree even when the tree was empty (NO_ID), leading to out of bounds accesses. The distance returned by the tree is now reused and NO_ID gives std::nullopt. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tion Stratigraphic location functions and AABB trees were only added with try_emplace, so entries of removed blocks (resp. surfaces) or of replaced meshes were kept forever: stratigraphic_bounding_box() threw on removed components and functions could reference deleted meshes. Both caches are now cleared before being rebuilt. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…omponent Stratigraphic coordinates were fetched through two hash map lookups per vertex in hot loops (AABB tree creation, distance callbacks evaluating 8 lookups per element, stratigraphic surfaces and lines). The location function and the implicit attribute are now resolved once per block (resp. surface) and reused. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tices stratigraphic_bounding_box() built a full stratigraphic AABB tree per block (resp. surface) only to read its root box, and threw on empty meshes. The box is now computed directly from the stratigraphic coordinates of the element vertices, empty components are ignored. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…_implicit_model_and_coords The stratigraphic locations were written in a new attribute with a random id, while the created StratigraphicModel uses its own stratigraphic location attribute: all locations were zero. They are now set through the StratigraphicModelBuilder once the model is created. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Stratigraphic coordinates are now computed once per vertex instead of once per tetrahedron vertex, a single summary is logged instead of one message per invalid tetrahedron, and blocks not meshed as TetrahedralSolid are skipped instead of throwing. Co-Authored-By: Claude Opus 5.5 <[email protected]>
StratigraphicSectionBuilder::copy never copied the stratigraphic locations: all the copied locations were zero. A copy_stratigraphic_attribute_values method, mirroring the 3D builder, now copies them and removes the stale attribute copied with the meshes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ations in copy copy_stratigraphic_attribute_values threw on blocks without stratigraphic location attribute (e.g. blocks not meshed as TetrahedralSolid) and kept the stale attribute copied with the meshes. Such blocks are now skipped and the copied attribute is removed once its values are transferred. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…obust import_old_stratigraphic_attribute_values_from_attribute_name threw on blocks (resp. surfaces) without the old attribute, could pick the model's own attribute when it shares the same name, never removed the old attribute in 3D and did not reset the stratigraphic query trees. Components without the old attribute are now skipped, the model's own attribute is never used as source nor deleted, old attributes are removed after import and the query trees are reinitialized. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…igning blocks to units complete_stratigraphic_unit_block_relationships dereferenced the optional units above/under horizons without checking them, throwing std::bad_optional_access for model horizons missing from the HorizonsStack or for top/bottom horizons without unit beyond them. Horizons missing from the stack are now ignored and missing units prevent the assignment instead of throwing. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…reated when assigning blocks complete_stratigraphic_unit_block_relationships removed, from a destructor also run during stack unwinding, every empty stratigraphic unit of the model, including the ones created by the user. Only the units created from the HorizonsStack and still empty are now removed, explicitly at the end of the assignment (or before rethrowing). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…rizon and an older erosion or a younger baselap When a block is bounded by one conformal horizon and one erosion (resp. baselap), the second case tested is_directly_above(above(conformal), erosion) (resp. is_directly_above(above(baselap), conformal)), which can never be true. Blocks between an erosion below and a conformal horizon above are now assigned to the unit under the conformal horizon, and blocks between a conformal horizon below and a baselap above to the unit above the conformal horizon, symmetrically to the existing cases. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…an two conformal horizons from an arbitrary one Blocks bounded by more than two conformal horizons were processed as if they had a single one, using an arbitrary conformal horizon with an erosion or a baselap and possibly assigning a wrong unit. Such ambiguous blocks are now only assigned through their neighbors. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…atigraphic units The fixed-point loop rescanned every block, and recollected the horizons of all its boundaries, at each pass until no block was assigned anymore, which is quadratic for propagation chains. Only the unassigned neighbors of newly assigned blocks are now reprocessed, assigned blocks are kept in a hash set and the horizon boundaries of each block are cached. Co-Authored-By: Claude Opus 5.5 <[email protected]>
clone() rebuilt the model from a cloned ImplicitStructuralModel, so the clone used a new stratigraphic location attribute (all zeros) next to the copied one with the same name. The copied values are now transferred to the clone attribute and the copied attribute is removed, for both StratigraphicModel and StratigraphicSection. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… a modified BRep The constructors from an initial model and a modified BRep (resp. Section) created a new stratigraphic location attribute (all zeros) next to the one carried by the new meshes. The locations stored with the initial model attribute id are now transferred and the stale attribute removed. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… blocks or stratigraphic units FaultBlocks and StratigraphicUnits were added after the first StructuralModel and CrossSection files were written: loading those files logged a missing file warning for each of these collections. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…thon builder methods pybind11 signatures of add_horizon_in_stratigraphic_unit referenced the C++ type name because InsertedHorizonInfo was registered afterwards, which made pybind11-stubgen fail when generating the wheel stubs. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Cpp-Linter Report
|
…eosciences into fix/robustness-performance-review
…oordinates test stratigraphic_coordinates() returns by value and stratigraphic_location() returns a reference into it: the test read a dangling reference, which failed with GCC in Debug. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…existing fault blocks Skipping Blocks already in a FaultBlock (c5ce8f9) broke Geode-DHI, which rebuilds the FaultBlocks next to the existing ones and compares both sets to detect splits and merges: the new set was empty and splits were missed. Restore the previous behavior and document it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Fixes from a robustness and performance review of the whole library. There are 67 commits. Each commit holds one fix and the test for it. Each test was checked to fail, hang or crash before its fix.
Behaviour changes to know about
HorizonsStackBuilder::set_horizon_above/set_horizon_underadd_above_relationthrows when the new relation would close a cycle. Cyclic.og_hstfiles are repaired on load, with a warning.StructuralModelBuilder::copy/CrossSectionBuilder::copyrequire an empty target model, likeBRepBuilder::copy.noexceptremoved from the converting constructors that can throw, for the explicit, implicit and stratigraphic models and sections. Non-triangulated surfaces and non-tetrahedral blocks are now skipped instead of throwing.OAMS_TRADITIONAL_GIS_ORDER).set_*_activefor geological components.StratigraphicSectionBuilder::copy_stratigraphic_attribute_values.add_horizon_in_stratigraphic_unitandcompute_top_and_bottom_horizons.GeographicCoordinateSystem::create_from_attribute, which was declared but never defined.Main fixes
containing_stratigraphic_unitlooped forever for any value between two horizons.clone(), in the(initial, BRep/Section, mappings)constructors, and when copying aStratigraphicSection.optional::value()in horizon ranges,string()and the blocks-to-units builder.Performance
invalid_stratigraphic_tetrahedracomputes coordinates once per vertex and logs one summary line.Transformcall.Checks
next.next, built against this branch, passes all 21 of its tests, including the Skua horizons stack test.🤖 Generated with Claude Code