Feature/gis2prof - Import Refactor - #156
Conversation
…h factory pattern
Dependabot and sonarcloud issues:Upgrade all packages with Add
Now failing |
There was a problem hiding this comment.
Pull request overview
This PR refactors FM2PROF’s import/initialisation pipeline to be source-agnostic (in preparation for GIS2PROF), introducing an importer factory + ModelData container and extracting runner preprocessing into a dedicated data_preprocessing module. It also adds a geometry-only CSV elevation importer and updates tests/fixtures accordingly.
Changes:
- Introduces
fm2prof.importspackage (factory + format-specific importers) and replaces legacyFMDataImporter/FmModelDatausage. - Extracts model-data initialisation/classification into
fm2prof.data_preprocessing.build_model_data. - Updates runner, polygon handling, docs, dependencies, and tests to use the new structures; adds new test data for the CSV elevation case.
Reviewed changes
Copilot reviewed 19 out of 29 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_data/cases/case_20_only_elevation/model/NetworkDefinition.ini | Adds 1D network definition fixture for elevation-only case. |
| tests/test_data/cases/case_20_only_elevation/model/CrossSectionLocations.ini | Adds cross-section locations fixture for elevation-only case. |
| tests/test_data_preprocessing.py | Adds preprocessing/classification test + cache cleanup fixture. |
| tests/test_data_import.py | Updates import tests to new importers/factory + adds CSV elevation importer tests. |
| tests/test_csv_elevation_importer.py | Added as a test module placeholder (currently empty). |
| tests/test_acceptance.py | Ensures polygon cache files are cleared before acceptance runs. |
| pyproject.toml | Updates dependency versions and adds pip-audit to dev deps. |
| fm2prof/polygon_file.py | Switches polygon mesh reading to ImporterFactory-created importer. |
| fm2prof/nearest_neighbour.py | Exposes get_class_tree publicly while keeping _get_class_tree alias. |
| fm2prof/ini_file.py | Adds InputFiles dataclass + IniFile.get_input_files(). |
| fm2prof/imports/factory.py | Adds ImporterFactory for source-based importer construction. |
| fm2prof/imports/dflowfm.py | Adds D-Flow FM importer returning ModelData. |
| fm2prof/imports/csv_elevation.py | Adds geometry-only CSV elevation importer returning ModelData. |
| fm2prof/imports/base.py | Adds typed geometry/hydraulic containers + ModelData and BaseImporter. |
| fm2prof/imports/init.py | Exposes public import API (ModelData, ImporterFactory, etc.). |
| fm2prof/fm2prof_runner.py | Delegates initialisation to build_model_data and renames fm_model_data → model_data. |
| fm2prof/data_preprocessing.py | New preprocessing pipeline for loading + classification. |
| fm2prof/data_import.py | Removes legacy importer/model container, leaving input-file parsing utilities. |
| fm2prof/cross_section.py | Renames fm_data → model_data key usage in cross-section generation. |
| docs/notebooks/cross_section_data.ipynb | Updates notebook to use model_data instead of fm_data. |
Comments suppressed due to low confidence (4)
fm2prof/data_preprocessing.py:155
- _classify_cross_sections_using_regions assumes edges are always present (model_data.edges.region / sclass). For geometry-only sources (edges=None) this will raise. Guard edge assignments behind ModelData.has_edges.
gridpoints_in_regions: GridPointsInPolygonResults = regions.get_gridpoints_in_polygon(input_files.map_file)
model_data.geometry.region = gridpoints_in_regions.faces_in_polygon
model_data.edges.region = gridpoints_in_regions.edges_in_polygon
css_regions = regions.get_points_in_polygon(cssdata["xy"], property_name="region")
model_data.geometry.sclass = model_data.geometry.region.copy()
model_data.edges.sclass = model_data.edges.region.copy()
fm2prof/data_preprocessing.py:169
- Inside the per-region loop, edge classification should also be conditional. As written it always computes edge_mask and writes to model_data.edges.sclass, which fails when edges are absent.
neigh = nearest_neighbour.get_class_tree(css_xy, css_id)
node_mask = model_data.geometry.region == region
model_data.geometry.sclass[node_mask] = neigh.predict(
np.array([model_data.geometry.x[node_mask], model_data.geometry.y[node_mask]]).T,
)
edge_mask = model_data.edges.region == region
model_data.edges.sclass[edge_mask] = neigh.predict(
np.array([model_data.edges.x[edge_mask], model_data.edges.y[edge_mask]]).T,
)
fm2prof/data_preprocessing.py:182
- _classify_to_sections assumes both hydraulics and edges exist when no section polygon is provided. This will crash for sources that intentionally omit edges/hydraulics (e.g. csv_elevation) and undermines the goal of supporting geometry-only workflows. Add has_edges/has_hydraulics guards and only assign edge sections when edges exist.
def _classify_to_sections(model_data: ModelData, sections: SectionPolygon, map_file: Path) -> ModelData:
"""Classify 2D faces and edges to roughness sections."""
if sections is None:
model_data.edges.section = classify_sections_by_variance(
model_data.edges.section, model_data.hydraulics.chezy_edge,
)
model_data.geometry.section = classify_sections_by_variance(
model_data.geometry.section, model_data.hydraulics.waterlevel,
)
fm2prof/imports/dflowfm.py:142
- EdgeGeometry classification fields are also initialised with dtype='U99'. For consistency with EdgeGeometry's dtype metadata (object) and to prevent truncation of long names, initialise these arrays with dtype=object as well.
# classification fields — populated later by fm2prof_runner
section = np.array(["main"] * n_edges, dtype="U99"),
region = np.array(["undefined"] * n_edges, dtype="U99"),
sclass = np.array([""] * n_edges, dtype="U99"),
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor of the Import functionality
This is a big 'un, and might be difficult to review. I've tried to lay down the main idea below.
Why this PR
In FM2PROF 2.6 we plan to add "GIS2PROF" functionality, which comes down to:
To lay the groundwork for this functionality, we need to do some serious plumbing work. Mainly:
time_independent_data). The new structure makes this explicit, in prep. for a new file format that does not have hydraulic or edge data.Finally, to lay proper groundwork, we'll:
dependabotandsonarcloudissues as we reasonably can.Design plan
exportfunctionality (when we added the D-HYDRO output format)._set_fm_model_data) out of the runner class and into adata_preprocessingmodule.Refactor 1: import into dedicated package
Summary
Refactors the import functionality from data_import.py into a dedicated imports package, following the same pattern as the existing export module.
New structure:
base.py— AbstractBaseImporterclass and generalizedModelData(replacingFmModelData)dflowfm.py—DFlowFMImportermigrated fromFMDataImporter, implementingimport_data() -> ModelDatafactory.py—ImporterFactory.create(source, file_path)for source-agnostic importer instantiation__init__.py— Public API exportsUpdated files:
fm2prof_runner.py— UsesImporterFactoryandModelDatainstead ofFMDataImporter/FmModelDatapolygon_file.py— UsesImporterFactory.create("dflowfm", ...)instead ofFMDataImporterdirectlyDFlowFMImporterandModelDataRefactor 2: source-agnostic
ModelDatapreprocessing pipelineSummary
Extracts all 2D model data loading and classification from
Fm2ProfRunnerinto a dedicated, testable data_preprocessing.py module. Geometry, edge and hydraulic dataclasses now enforce correct numpy dtypes on every field assignment, preventing silent string truncation bugs.Changes
New: data_preprocessing.py
build_model_data(input_files, ...)as the single entry point for the initialisation pipelineImporterFactory, making it straightforward to support future input formatsNew: ini_file.py —
InputFilesdataclassmap_file,css_file,region_fileandsection_fileIniFile.get_input_files()constructs and returns anInputFilesinstance, normalising optional files toNoneUpdated: base.py
FaceGeometry,EdgeGeometryandHydraulicDatadeclare dtype per field usingfield(metadata={"dtype": ...})_ndarray_setattrreads the metadata and callsnp.asarray(value, dtype=dtype)on every assignment — including post-construction mutationsection,region,sclass) usedtype=object, preventing numpy's fixed-width<UNtruncation of variable-length cross-section namesUpdated: fm2prof_runner.py
_initialise_fm2profdelegates entirely tobuild_model_data; legacy source-specific code removedini_file.get_input_files()instead of four separateget_input_file()callsfm_model_datato source-agnosticmodel_datavariable namesUpdated: cross_sections.py
fm_model_datato source-agnosticmodel_datavariable namesNew: test_data_preprocessing.py
TestClassification.test_region_polygon_assigns_all_faces_to_poly1verifies that all 2D faces are assigned to regionpoly1for the compound-with-region acceptance caseUpdated: test_acceptance.py
clear_polygon_cachesautouse fixture deletes*_cache.jsonfiles before each test, ensuring fresh polygon classification on every run3. Upgrade packages and security issues
Upgraded all packages with
uv lock --upgradeAdd
pip-auditto dev depencies, runuv run pip-auditpytest- upgrade to >= 9.0Upgrading caused failing
test_clitests, becauseclickupgraded pasttyper. Upgraded thetyperdependency inpyproject.toml