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

Feature/unst 8490 621 extforce converter boundary condition #717

Conversation

MAfarrag
Copy link

@MAfarrag MAfarrag commented Nov 25, 2024

Add Converter for the Boundary Condition quantities in the old external forcing file

New Features

Now there is a boundary condition converter

Design

  • All the existing converters "initial condition, boundary condition, mete data" now exist in one file hydrolib/tools/ext_old_to_new/converters.py as there are a lot of similarities and most probably in the future they will be redesigned, or lots of the repeated lines will be moved upstream to the BaseConverter super class.
  • The ext_old_to_new is redesigned and replaced with the ExternalForcingConverter class
ExternalForcingConverter
|-__init__
|- read_old_file
|- update
|- save
|- extold_model
|- ext_model (setter and getter)
|- inifield_model (setter and getter)
|- structure_model (setter and getter)
  • The backup parameter in the hydrolib/tools/ext_old_to_new/utils.backup_file funcion is remove as there is now need to enter the function if you actually do not want to backup you file (the function should be called only if you need its functionality) and the condition should be made in the calling script.

Formating and Styling

  • I added lots of formatters that helps to format your code locally before pushing to the sonar online tool, these formatter works standalone or with pre-commit hooks.
  • The pre-commit hooks configuration file is not part of the repo for now.

Testing

Data

  • tests/data/input/boundary-conditions/old-external-boundary_condition_only.ext
  • tests/data/input/dflowfm_individual_files/polylines/polyline-no-z-no-label.pli
  • tests/data/input/dflowfm_individual_files/polylines/polyline-no-z-with-label.pli
  • `tests/data/input/dflowfm_individual_files/polylines/polyline-with-z-no-label.pli``
  • tests/data/input/dflowfm_individual_files/polylines/polyline-with-z-no-label.pliz

New Tests

  • Creating subfolders in the testing directory to replicate the same subfolders/modules of the package. tests/dflowfm/extold/test_boundary.py .
  • Create tests/dflowfm/extold/test_boundary.py::TestBoundary::test_existing_file.
  • Test the polyline with different cases tests/dflowfm/polyfile/test_polyline_models.py, currently the polyline failes with the scientific notation, and reads only the the number of dimensions (2*2) will read only the first two numbers in the row even if there is a z, and the z value will only be read if the second dimension is 3 and the extension has to be .pliz in the same time.
  • create a test for the converter tests/tools/test_converters.py::TestBoundaryConverter.
  • create tests for all the methods and properties in the ExternalForcingConverter class.

Old errors

  • Multiple uses of mutable objects like lists [] as a default values was converted to Field(default_factory=list)

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, see my various comments. Maybe we need to talk tomorrow about the (silly) difference between regular polylines and boundary polylines.

hydrolib/tools/ext_old_to_new/converters.py Show resolved Hide resolved
hydrolib/tools/ext_old_to_new/converters.py Show resolved Hide resolved
hydrolib/tools/ext_old_to_new/converters.py Show resolved Hide resolved
hydrolib/tools/ext_old_to_new/converters.py Show resolved Hide resolved
hydrolib/tools/ext_old_to_new/converters.py Outdated Show resolved Hide resolved
hydrolib/tools/ext_old_to_new/main_converter.py Outdated Show resolved Hide resolved
hydrolib/tools/ext_old_to_new/main_converter.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
MAfarrag and others added 19 commits December 9, 2024 17:34
…of github.com:Deltares/HYDROLIB-core into feature/621_extforce_converter-initial-condition-file
…del` or a path to external forcing file and the `_read_old_file` not is used inside the constructor to read the external forcing file if path is given to the converter
Copy link
Author

@MAfarrag MAfarrag left a comment

Choose a reason for hiding this comment

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

.

Base automatically changed from feature/621_extforce_converter-initial-condition-file to feature/621_extforce_converter December 12, 2024 13:07
@MAfarrag
Copy link
Author

MAfarrag commented Jan 6, 2025

all the comments here are already done

@MAfarrag MAfarrag merged commit b2e22aa into feature/621_extforce_converter Jan 7, 2025
11 checks passed
@MAfarrag MAfarrag deleted the feature/unst-8490-621_extforce_converter-boundary-condition branch January 7, 2025 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high type: feature Brand new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants