Skip to content

Feature/gis2prof - Import Refactor - #156

Open
kdberends wants to merge 10 commits into
dev/2.6.0from
feature/gis2prof
Open

Feature/gis2prof - Import Refactor#156
kdberends wants to merge 10 commits into
dev/2.6.0from
feature/gis2prof

Conversation

@kdberends

@kdberends kdberends commented Jul 29, 2026

Copy link
Copy Markdown
Member

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:

  • supporting a new input format besides dflowfm
  • changing the workflow to allow for cross-section generation without hydraulic data

To lay the groundwork for this functionality, we need to do some serious plumbing work. Mainly:

  1. Rewriting the import functionality such that it supports multiple formats without code duplication, and remove hardcoded references to dfm which are littered throughout the code
  2. Clean-up the model-data class such that is much more clear what type of data is required when and if we have a format that's missing some stuff - and make it more testable.
  • The current implementation mixed data loading, I/O and classification logic directly inside the runner class, making it hard to test classification in isolation, so we should also take the data pre-processing out of the runner class into a separate package.
  • The current implementation was less explicit about what kinds of data were necessary and the type of that data (e.g. 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:

  • tackle as much dependabot and sonarcloud issues as we reasonably can.

Design plan

  1. Adopt a factory-architecture like we did with the refactor of the export functionality (when we added the D-HYDRO output format).
  2. (a) Rewrite FM_Model_Data into source-agnostic Model_Data with proper typing, type-checking and explicit coercing, insofar python allows us to do this. (b) yank the data preprocessing (_set_fm_model_data) out of the runner class and into a data_preprocessing module.

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 — Abstract BaseImporter class and generalized ModelData (replacing FmModelData)
  • dflowfm.pyDFlowFMImporter migrated from FMDataImporter, implementing import_data() -> ModelData
  • factory.pyImporterFactory.create(source, file_path) for source-agnostic importer instantiation
  • __init__.py — Public API exports

Updated files:

  • fm2prof_runner.py — Uses ImporterFactory and ModelData instead of FMDataImporter/FmModelData
  • polygon_file.py — Uses ImporterFactory.create("dflowfm", ...) instead of FMDataImporter directly
  • test_data_import.py — Updated to use DFlowFMImporter and ModelData

Refactor 2: source-agnostic ModelData preprocessing pipeline

Summary

Extracts all 2D model data loading and classification from Fm2ProfRunner into 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

  • Introduces build_model_data(input_files, ...) as the single entry point for the initialisation pipeline
  • Source-agnostic: format is selected via ImporterFactory, making it straightforward to support future input formats
  • Handles region/section polygon classification, nearest-neighbour cross-section assignment, and Chézy variance-based section splitting

New: ini_file.py — InputFiles dataclass

  • Convenience container grouping map_file, css_file, region_file and section_file
  • IniFile.get_input_files() constructs and returns an InputFiles instance, normalising optional files to None
  • Callers no longer pass four individual path arguments

Updated: base.py

  • FaceGeometry, EdgeGeometry and HydraulicData declare dtype per field using field(metadata={"dtype": ...})
  • Shared _ndarray_setattr reads the metadata and calls np.asarray(value, dtype=dtype) on every assignment — including post-construction mutation
  • String fields (section, region, sclass) use dtype=object, preventing numpy's fixed-width <UN truncation of variable-length cross-section names

Updated: fm2prof_runner.py

  • _initialise_fm2prof delegates entirely to build_model_data; legacy source-specific code removed
  • Uses ini_file.get_input_files() instead of four separate get_input_file() calls
  • switched from fm_model_data to source-agnostic model_data variable names

Updated: cross_sections.py

  • switched from fm_model_data to source-agnostic model_data variable names

New: test_data_preprocessing.py

  • TestClassification.test_region_polygon_assigns_all_faces_to_poly1 verifies that all 2D faces are assigned to region poly1 for the compound-with-region acceptance case

Updated: test_acceptance.py

  • clear_polygon_caches autouse fixture deletes *_cache.json files before each test, ensuring fresh polygon classification on every run

3. Upgrade packages and security issues

  • Upgraded all packages with uv lock --upgrade

  • Add pip-audit to dev depencies, run uv run pip-audit

    • Remaining vulnerability in pytest - upgrade to >= 9.0

Upgrading caused failing test_cli tests, because click upgraded past typer. Upgraded the typer dependency in pyproject.toml

@kdberends

kdberends commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

Dependabot and sonarcloud issues:

Upgrade all packages with uv lock --upgrade
deltares-hms-docs = { git = "https://github.com/Deltares-research/doc_utils.git", branch="latex-templates"} no longer exists on remote, so switch to the main branch.

Add pip-audit to dev depencies, run pip-audit

  • Remaining vulnerability in pytest - upgrade to >= 9.0

Now failing test_cli versions, because the click upgraded past the typer. Should upgrade the typer dependency in the package too.

@kdberends kdberends mentioned this pull request Jul 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.imports package (factory + format-specific importers) and replaces legacy FMDataImporter/FmModelData usage.
  • 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_datamodel_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_datamodel_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.

Comment thread fm2prof/data_preprocessing.py
Comment thread fm2prof/data_preprocessing.py
Comment thread fm2prof/imports/dflowfm.py Outdated
Comment thread fm2prof/imports/dflowfm.py
Comment thread fm2prof/imports/base.py
Comment thread fm2prof/fm2prof_runner.py
Comment thread fm2prof/fm2prof_runner.py
Comment thread fm2prof/fm2prof_runner.py
@kdberends
kdberends requested a review from MAfarrag July 29, 2026 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants