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

T3D header not correct #356

Merged
merged 48 commits into from
Oct 14, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
d47cf4e
Added ZSurf option for verticalPositionType.
tim-vd-aardweg Sep 29, 2022
1b48656
Added a child class that inherrits from QuantityUnitPair that also ha…
tim-vd-aardweg Sep 29, 2022
7282233
Fixed the aliases to adhere to the 2D3D manual.
tim-vd-aardweg Sep 29, 2022
0e70a0a
Reverted Time Interpolation back to the original timeInterpolation. U…
tim-vd-aardweg Sep 29, 2022
1a1163e
Added support for the timeInterpolation keyword when creating a T3D f…
tim-vd-aardweg Sep 29, 2022
096b2b8
Removed the QuantityUnitPositionPair class again, in favour of a bett…
tim-vd-aardweg Sep 30, 2022
4164906
Fixed failing tests as a result of making the QuantityUnitPair a Base…
tim-vd-aardweg Oct 3, 2022
105c9e8
Added new test for the QuantityUnitPair.
tim-vd-aardweg Oct 3, 2022
1c05236
Added documentation to the VerticalInterpolation enum.
tim-vd-aardweg Oct 3, 2022
15499a9
Added documentation to the TimeInterpolation enum.
tim-vd-aardweg Oct 3, 2022
91ddeef
Added documentation to the QuantityUnitPair class.
tim-vd-aardweg Oct 3, 2022
ea938a7
Updated documentation of ForcingBase to the new documentation style.
tim-vd-aardweg Oct 3, 2022
5d691e0
Added documenation to the T3D class.
tim-vd-aardweg Oct 3, 2022
c37deeb
Added validation: Ensure the first QuantityUnitPair is for `time`.
tim-vd-aardweg Oct 3, 2022
7d7a6cb
Intermediate commit of the new validation.
tim-vd-aardweg Oct 3, 2022
b417154
Intermediate commit of the new validation.
tim-vd-aardweg Oct 3, 2022
c50c71f
Fixed an error in the validator and fixed formatting.
tim-vd-aardweg Oct 3, 2022
735a3c8
Fixed some testcases and added a couple of testcases.
tim-vd-aardweg Oct 3, 2022
1a10026
Added additional testcases
tim-vd-aardweg Oct 4, 2022
bbde372
Added additional testcases
tim-vd-aardweg Oct 4, 2022
8d4d6f3
Added additional test cases.
tim-vd-aardweg Oct 4, 2022
fdebf78
Updated reference data for T3D
tim-vd-aardweg Oct 4, 2022
3a44fe3
Updated the remaining documentation for this file.
tim-vd-aardweg Oct 4, 2022
82b386a
autoformat: isort & black
tim-vd-aardweg Oct 4, 2022
3e24262
Fixed a bug with the `number_of_verticalpositions`. Implemented revie…
tim-vd-aardweg Oct 6, 2022
2cb275a
Use `Vertical Position` instead of `verticalpositionindex`
tim-vd-aardweg Oct 7, 2022
83c11f0
Updated keywords for the T3D header and added a test to ensure loadin…
tim-vd-aardweg Oct 7, 2022
87e78bd
Fixed code smells.
tim-vd-aardweg Oct 10, 2022
0694680
Fixed code smells.
tim-vd-aardweg Oct 10, 2022
f140d80
Add failing testcase that should pass
tim-vd-aardweg Oct 11, 2022
93e4f4b
Added a way to support the 'Vertical Position Specification' keyword …
tim-vd-aardweg Oct 11, 2022
aee2917
autoformat: isort & black
tim-vd-aardweg Oct 11, 2022
72c8d49
Merge branch 'main' into fix/317_t3d_header_not_correct
priscavdsluis Oct 12, 2022
0e1048a
Updated the keywords to the new keywords.
tim-vd-aardweg Oct 13, 2022
864ce5b
Merge branch 'fix/317_t3d_header_not_correct' of https://github.com/D…
tim-vd-aardweg Oct 13, 2022
5489c15
Updated error message to be more clear
tim-vd-aardweg Oct 13, 2022
88fb9fc
Added backwards compatibility for the keywords in the .bc file that u…
tim-vd-aardweg Oct 13, 2022
64b972c
autoformat: isort & black
tim-vd-aardweg Oct 13, 2022
f64a069
Added backwards compatibility for the `Time Interpolation` keyword in…
tim-vd-aardweg Oct 13, 2022
ae1d1a4
Added error when the user forgot to specify the vertpositions field t…
tim-vd-aardweg Oct 13, 2022
2e6bc19
Fixed the default enum values for timeinterpolation and verticalinter…
tim-vd-aardweg Oct 13, 2022
68a6cc5
Removed the check for this unit as it causes problems on the build se…
tim-vd-aardweg Oct 13, 2022
49c1575
Removed the ForcingBackwardsCompatibilityHelper and instead added a g…
tim-vd-aardweg Oct 13, 2022
dd24bf0
autoformat: isort & black
tim-vd-aardweg Oct 13, 2022
9f55295
Update hydrolib/core/io/bc/models.py
arthurvd Oct 13, 2022
5a9339a
Implemented review comments.
tim-vd-aardweg Oct 14, 2022
6c436bd
Merge branch 'main' into fix/317_t3d_header_not_correct
priscavdsluis Oct 14, 2022
0061257
autoformat: isort & black
priscavdsluis Oct 14, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 18 additions & 118 deletions hydrolib/core/io/bc/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

"""
import logging
from ast import For
from enum import Enum
from pathlib import Path
from typing import Callable, Dict, List, Literal, Optional, Set, Union
Expand All @@ -30,6 +29,7 @@
from hydrolib.core.io.ini.util import (
get_enum_validator,
get_from_subclass_defaults,
get_key_renaming_root_validator,
get_split_string_on_delimiter_validator,
make_list_validator,
)
Expand Down Expand Up @@ -100,85 +100,6 @@ def _to_properties(self):
yield Property(key="vertPositionIndex", value=self.vertpositionindex)


class ForcingBackwardsCompatibilityHelper:
"""Class to add backwards compatibility for certain keywords in the .bc file."""

@staticmethod
def add_backwards_compatibility_for_timeinterpolation(values: Dict) -> Dict:
"""Add backwards compatibility for older timeInterpolation keywords."""

timeinterpolation_keyword = "timeinterpolation"

if values.get(timeinterpolation_keyword) is not None:
return values

old_keyword = "time_interpolation"
if values.get(old_keyword) is not None:
values[timeinterpolation_keyword] = values.get(old_keyword)

return values

@staticmethod
def add_backwards_compatibility_for_vertpositions(values: Dict) -> Dict:
"""Add backwards compatibility for older vertPositions keywords."""

vertpositions_keyword = "vertpositions"

if values.get(vertpositions_keyword) is not None:
return values

old_keyword = "vertical_position_specification"
if values.get(old_keyword) is not None:
values[vertpositions_keyword] = values.get(old_keyword)

return values

@staticmethod
def add_backwards_compatibility_for_vertinterpolation(values: Dict) -> Dict:
"""Add backwards compatibility for older vertInterpolation keywords."""

vertinterpolation_keyword = "vertinterpolation"

if values.get(vertinterpolation_keyword) is not None:
return values

old_keyword = "vertical_interpolation"
if values.get(old_keyword) is not None:
values[vertinterpolation_keyword] = values.get(old_keyword)

return values

@staticmethod
def add_backwards_compatibility_for_vertpositiontype(values: Dict) -> Dict:
"""Add backwards compatibility for older vertPositionType keywords."""

vertpositiontype_keyword = "vertpositiontype"

if values.get(vertpositiontype_keyword) is not None:
return values

old_keyword = "vertical_position_type"
if values.get(old_keyword) is not None:
values[vertpositiontype_keyword] = values.get(old_keyword)

return values

@staticmethod
def add_backwards_compatibility_for_vertpositionindex(values: Dict) -> Dict:
"""Add backwards compatibility for older vertPositionIndex keywords."""

vertpositionindex_keyword = "vertpositionindex"

if values.get(vertpositionindex_keyword) is not None:
return values

old_keyword = "vertical_position"
if values.get(old_keyword) is not None:
values[vertpositionindex_keyword] = values.get(old_keyword)

return values


class ForcingBase(DataBlockINIBasedModel):
"""
The base class of a single [Forcing] block in a .bc forcings file.
Expand Down Expand Up @@ -307,13 +228,11 @@ class TimeSeries(ForcingBase):
"timeinterpolation", enum=TimeInterpolation
)

@root_validator(pre=True)
def add_backwards_compatibility_for_timeinterpolation(cls, values: Dict) -> Dict:
ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_timeinterpolation(
values
)

return values
_key_renaming_root_validator = get_key_renaming_root_validator(
{
"timeinterpolation": ["time_interpolation"],
}
)


class Harmonic(ForcingBase):
Expand Down Expand Up @@ -373,6 +292,16 @@ class T3D(ForcingBase):
)
"""TimeInterpolation: The type of time interpolation. Defaults to linear."""

_key_renaming_root_validator = get_key_renaming_root_validator(
{
"timeinterpolation": ["time_interpolation"],
"vertpositions": ["vertical_position_specification"],
priscavdsluis marked this conversation as resolved.
Show resolved Hide resolved
"vertinterpolation": ["vertical_interpolation"],
"vertpositiontype": ["vertical_position_type"],
"vertpositionindex": ["vertical_position"],
}
)

_split_to_list = get_split_string_on_delimiter_validator(
"vertpositions",
)
Expand All @@ -388,36 +317,7 @@ class T3D(ForcingBase):
)

@root_validator(pre=True)
def _add_support_for_keywords_with_spaces_and_validate_quantityunitpair(
cls, values: Dict
) -> Dict:
values = cls._add_support_for_keywords_with_spaces(values)
values = cls._validate_quantityunitpair(values)

return values

@classmethod
def _add_support_for_keywords_with_spaces(cls, values: Dict) -> Dict:
ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_timeinterpolation(
values
)
ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_vertinterpolation(
values
)
ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_vertpositionindex(
values
)
ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_vertpositions(
values
)
ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_vertpositiontype(
values
)

return values

@classmethod
def _validate_quantityunitpair(cls, values: Dict) -> Dict:
def _validate_quantityunitpairs(cls, values: Dict) -> Dict:
super()._validate_quantityunitpair(values)

quantityunitpairs = values["quantityunitpair"]
Expand Down Expand Up @@ -484,7 +384,7 @@ def _validate_verticalpositionindexes_and_update_quantityunitpairs(
quantityunitpairs: List[QuantityUnitPair],
) -> None:
if verticalpositionindexes is None:
raise ValueError("vertpositionindex is not provided")
raise ValueError("vertPositionIndex is not provided")

if len(verticalpositionindexes) != len(quantityunitpairs) - 1:
raise ValueError(
Expand Down
25 changes: 25 additions & 0 deletions hydrolib/core/io/ini/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -462,3 +462,28 @@ def is_valid_coordinates_with_num_coordinates_specification() -> bool:
raise ValueError(error)

return root_validator(allow_reuse=True)(validate_location_specification)


def get_key_renaming_root_validator(keys_to_rename: Dict[str, List[str]]):
Copy link
Member

Choose a reason for hiding this comment

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

Yes! Like this a lot :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! :)

priscavdsluis marked this conversation as resolved.
Show resolved Hide resolved
"""
Gets a root validator that renames the provided keys to support backwards compatibility.

Args:
keys_to_rename Dict[str, List[str]]: Dictionary of keys and a list of old keys that
should be converted to the current key.
"""

def rename_keys(cls, values: Dict) -> Dict:
for current_keyword, old_keywords in keys_to_rename.items():
if values.get(current_keyword) is not None:
priscavdsluis marked this conversation as resolved.
Show resolved Hide resolved
continue

for old_keyword in old_keywords:
if values.get(old_keyword) is not None:
values[current_keyword] = values.get(old_keyword)
priscavdsluis marked this conversation as resolved.
Show resolved Hide resolved
del values[old_keyword]
break

return values

return root_validator(allow_reuse=True, pre=True)(rename_keys)
41 changes: 41 additions & 0 deletions tests/io/ini/test_util.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
from typing import Dict, List, Optional

import pytest
from pydantic import Extra
from pydantic.error_wrappers import ValidationError

from hydrolib.core.basemodel import BaseModel
from hydrolib.core.io.ini.util import (
LocationValidationConfiguration,
LocationValidationFieldNames,
get_key_renaming_root_validator,
get_location_specification_rootvalidator,
)

Expand Down Expand Up @@ -198,3 +200,42 @@ def test_correct_1d_fields_locationtype_is_added(
values
)
assert validated_values == expected_values


class TestGetKeyRenamingRootValidator:
class DummyModel(BaseModel):
"""Dummy model to test the validation of the location specification."""

randomproperty: str

validator = get_key_renaming_root_validator(
{
"randomproperty": [
"randomProperty",
"random_property",
"oldRandomProperty",
],
}
)

class Config:
extra = Extra.allow

@pytest.mark.parametrize(
"old_key", ["randomProperty", "random_property", "oldRandomProperty"]
)
def test_old_keys_are_correctly_renamed_to_current_keyword(self, old_key: str):
values = {old_key: "randomString"}

model = TestGetKeyRenamingRootValidator.DummyModel(**values)

assert model.randomproperty == "randomString"

def test_unknown_key_still_raises_error(self):
values = {"randomKeyThatNeverExisted": "randomString"}

with pytest.raises(ValidationError) as error:
TestGetKeyRenamingRootValidator.DummyModel(**values)

expected_message = "field required"
assert expected_message in str(error.value)
70 changes: 1 addition & 69 deletions tests/io/test_bc.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
Astronomic,
AstronomicCorrection,
Constant,
ForcingBackwardsCompatibilityHelper,
ForcingBase,
ForcingModel,
Harmonic,
Expand Down Expand Up @@ -580,7 +579,7 @@ def test_load_forcing_model(self):
[120.0, 7.0, 8.0, 9.0],
]

def test_load_t3d_model_with_old_keyword_that_contain_spaces(self):
def test_load_t3d_model_with_old_keyword_that_contains_spaces(self):
bc_file = Path(test_reference_dir / "bc" / TEST_BC_FILE_KEYWORDS_WITH_SPACES)
forcingmodel = ForcingModel(bc_file)

Expand Down Expand Up @@ -633,73 +632,6 @@ def _validate_that_correct_quantityunitpairs_are_created(
)


class TestForcingBackwardsCompatibilityHelper:
def test_old_timeinterpolation_keyword_gets_converted_to_current_keyword(self):
old_keyword = "time_interpolation"
data = TimeInterpolation.block_from

values = {old_keyword: data}

values = ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_timeinterpolation(
values
)

current_keyword = "timeinterpolation"
assert values[current_keyword] == data

def test_old_vertpositions_keyword_gets_converted_to_current_keyword(self):
old_keyword = "vertical_position_specification"
data = [1, 2, 3]

values = {old_keyword: data}

values = ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_vertpositions(
values
)

current_keyword = "vertpositions"
assert values[current_keyword] == data

def test_old_vertinterpolation_keyword_gets_converted_to_current_keyword(self):
old_keyword = "vertical_interpolation"
data = VerticalInterpolation.block

values = {old_keyword: data}

values = ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_vertinterpolation(
values
)

current_keyword = "vertinterpolation"
assert values[current_keyword] == data

def test_old_vertpositiontype_keyword_gets_converted_to_current_keyword(self):
old_keyword = "vertical_position_type"
data = VerticalPositionType.percentage_bed

values = {old_keyword: data}

values = ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_vertpositiontype(
values
)

current_keyword = "vertpositiontype"
assert values[current_keyword] == data

def test_old_vertpositionindex_keyword_gets_converted_to_current_keyword(self):
old_keyword = "vertical_position"
data = [1, 2, 3]

values = {old_keyword: data}

values = ForcingBackwardsCompatibilityHelper.add_backwards_compatibility_for_vertpositionindex(
values
)

current_keyword = "vertpositionindex"
assert values[current_keyword] == data


def _create_time_series_values():
return dict(
name="boundary_timeseries",
Expand Down