Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: sources sinks converter (UNST-8528) #721

Merged

Conversation

MAfarrag
Copy link

@MAfarrag MAfarrag commented Dec 12, 2024

Merge Topic

Sources and Sinks Converter

  • Add Source and Sinks converter
  • The sources and Sinks class SourceSink overrides the super class method of validating input parameters (quantities) to include initialtracers, tracebnd, sedfracbnd, initialsedfrac, and adds these quantities dynamically through the __init__ method in the class and the class method _exclude_from_validation
  • The superclass INIBasedModel is modified to allow certain keywords (quantities) to bypass the validation to allow the SourceSink class to implement the dynamic parameters/keywords above.
  • add functionality to the PolyFile class to retrieve the x, y and z_source_sinks coordinates.
  • create abstraction function convert_interpolation_data to create a dictionaly keys for the interpolation data for all converters.
  • add the verbose, fm_model, root_dir properties to the ExternalForcingConverter Converter class
  • add from_mdu method to create the converter from mdu file
  • the backup functionality in the save method adds the .bak after the file name and the extension file-name.txt becomes file-name.txt.bak
  • remove the post_fix as if conflict with the backup functionality.

Documentation

  • add full docstring to the PolyFile class including different cases of the polyline files

Dev

  • add tqdm pachage for progress bar visualiation

Testing

Data

  • input/
    • dflowfm_individual_files/
      • polylines/
        • boundary-polyline-with-z-no-label-2by3.pliz
        • leftsor-5-columns.pliz
    • source-sink/
      • leftsor-5-columns.pliz
      • leftsor.pliz
      • leftsor.tim
      • no_temperature_no_salinity.tim
      • no_temperature_or_salinity.tim
      • source-sink.ext
      • tim-3-columns.tim

tests

Tests in tools/test_converters.py

  • TestConvertParameters

    • test_sample_data_file
  • TestConvertMeteo

    • test_default
  • TestBoundaryConverter

    • test_default

to run in the terminal paste the following lines individually after pytest -vvv

  • tests/tools/test_converters.py::TestConvertParameters::test_sample_data_file
  • tests/tools/test_converters.py::TestConvertMeteo::test_default
  • tests/tools/test_converters.py::TestBoundaryConverter::test_default

Tests in tools/test_converters_source_sink.py

  • TestParseTimFileForSourceSink

    • test_default
    • test_list_of_ext_quantities_tim_column_mismatch
    • test_no_temperature
    • test_no_salinity
    • test_no_salinity_no_temperature
  • TestSourceSinkConverter

    • test_default
    • test_4_5_columns_polyline
    • test_no_temperature_no_salinity

to run in the terminal paste the following lines individually after pytest -vvv

  • tests/tools/test_converters_source_sink.py::TestParseTimFileForSourceSink::test_default
  • tests/tools/test_converters_source_sink.py::TestParseTimFileForSourceSink::test_list_of_ext_quantities_tim_column_mismatch
  • tests/tools/test_converters_source_sink.py::TestParseTimFileForSourceSink::test_no_temperature
  • tests/tools/test_converters_source_sink.py::TestParseTimFileForSourceSink::test_no_salinity
  • tests/tools/test_converters_source_sink.py::TestParseTimFileForSourceSink::test_no_salinity_no_temperature
  • tests/tools/test_converters_source_sink.py::TestSourceSinkConverter::test_default
  • tests/tools/test_converters_source_sink.py::TestSourceSinkConverter::test_4_5_columns_polyline
  • tests/tools/test_converters_source_sink.py::TestSourceSinkConverter::test_no_temperature_no_salinity

Tests in tools/test_main_converters.py

  • TestExtOldToNewFromMDU
    • test_wind_combi_uniform_curvi

to run in the terminal paste the following lines individually after pytest -vvv

  • tests/tools/test_main_converters.py::TestExtOldToNewFromMDU::test_wind_combi_uniform_curvi

Tests in tests/dflowfm/polyfile/test_polyline_models.py

  • TestGetZSourcesSinks
    • test_get_z_sources_sinks_no_z_values
      to run in the terminal paste the following lines individually after pytest -vvv
      tests/dflowfm/polyfile/test_polyline_models.py::TestGetZSourcesSinks.test_get_z_sources_sinks_no_z_values

Rearrange tests

  • tests/dflowfm/extold/test_ext_forcing.py and tests/dflowfm/extold/test_extold.py are spin off the tests/dflowfm/test_extold.py (moved to a subdirectory and split the file for better debugging)
  • the testing of the sources and sinks converter is mocked using the unittest methods patch and mock to avoid adding too many files (two files -pliz and tim - for each case), so the mock tricks the functions to read pair of pliz and tim files that does not have the same name.

Base automatically changed from feature/unst-8490-621_extforce_converter-boundary-condition to feature/621_extforce_converter January 7, 2025 09:49
@MAfarrag MAfarrag changed the title Feature/unst 8528 sources sinks converter feat: sources sinks converter (UNST-8528) Jan 7, 2025
Copy link
Member

@arthurvd arthurvd left a comment

Choose a reason for hiding this comment

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

See my individual remarks. I will review still a bit more in depth too.

hydrolib/core/dflowfm/ext/models.py Outdated Show resolved Hide resolved
hydrolib/core/dflowfm/ext/models.py Outdated Show resolved Hide resolved
hydrolib/core/dflowfm/ext/models.py Show resolved Hide resolved
hydrolib/core/dflowfm/ext/models.py Show resolved Hide resolved
hydrolib/core/dflowfm/ext/models.py Outdated Show resolved Hide resolved
hydrolib/core/dflowfm/polyfile/models.py Show resolved Hide resolved
hydrolib/tools/ext_old_to_new/converters.py Outdated Show resolved Hide resolved
hydrolib/tools/ext_old_to_new/converters.py Outdated Show resolved Hide resolved
hydrolib/tools/ext_old_to_new/converters.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
Copy link
Member

@arthurvd arthurvd left a comment

Choose a reason for hiding this comment

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

Hi @MAfarrag, thanks for all the fixes. Only a few minor typing thingies left. Almost ready to merge.

hydrolib/core/dflowfm/bc/models.py Outdated Show resolved Hide resolved
hydrolib/core/dflowfm/ext/models.py Show resolved Hide resolved
hydrolib/core/dflowfm/polyfile/models.py Show resolved Hide resolved
@MAfarrag MAfarrag requested a review from arthurvd January 13, 2025 17:43
…ure, and salinity in the `SourceSink` class"

This reverts commit 89b55a0

Signed-off-by: Mostafa Farrag <[email protected]>
Copy link
Member

@arthurvd arthurvd left a comment

Choose a reason for hiding this comment

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

As discussed: this part of the source sink converter is now OK.
The tim to BC converter part will continue in a separate issue + branch (UNST-8553).

@MAfarrag MAfarrag merged commit 24ee203 into feature/621_extforce_converter Jan 15, 2025
11 checks passed
@MAfarrag MAfarrag deleted the feature/unst-8528-sources-sinks-converter branch January 15, 2025 14:14
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