feat: external forcings converter #647
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* basic program structure * processing of a single [Meteo] block as first example code * testcase c081 with multiple combined wind files.
arthurvd
commented
Jun 6, 2024
There was a problem hiding this comment.
@priscavdsluis / @tim-vd-aardweg : what do you guys think about this extra "legacy.py" inside the MDU directory?
As long as we don't have versioned support for several old/newer formats of some of our filetypes, this was the solution I came up with.
There was a problem hiding this comment.
@arthurvd / @priscavdsluis
I am not entirely sure to be honest. My first question is: Do we want tools in HYDROLIB-core, or do we create a separate package for tools that use HYDROLIB-core?
But let's assume we do want to add tools to the HYDROLIB-core package. I think my next question is: Should we implement this 'legacy' model into hydrolib, or should it be defined in the tool that uses it? Also, the change you request touches on a broader topic that should be discussed: How do we handle old/deprecated files/sections/keywords. Your requested change is due to the fact that we cannot handle older/unexpected file formats. And your change fixes things for your specific use case.
Having said all that, my thoughts are as follows:
- We will need to generically support older/unexpected formats. That way, we don't need your specific legacy model.
- Since that probably won't happen in the short term, we need a different solution for your use case. My preference would be to add the LegacyModel to the specific tool, so we do not pollute the main HYDROLIB-core package with quick fixes for specific tools.
There was a problem hiding this comment.
Hi @arthurvd / @tim-vd-aardweg / @priscavdsluis , I know you didn't ask my opinion but I still want to ask some questions regarding this.
Previously I remember a presentation was given with a picture showing hydrolib-core and hydrolib.
And how it should be used by tools.
See https://www.deltares.nl/expertise/projecten/hydrolib-gezamenlijke-automatisering-op-watersysteemmodellen
Should the converter not be a tool build on top of hydrolib-core?
Especially if the support for the old file format is dropped and no longer backwards compatible supported by the kernels.
Should hydrolib-core still support the old format, or only the new format from version x?
…ing a meteo block.
…ne caused the converter to crash
…ties than only Meteo See ConverterFactory and BaseConverter subclasses ext_old_to_new is now a subdirectory, instead of a single script, to keep things organized. BREAKING CHANGE: in hydrolib.core: renamed ExtOldFileType.Poyfile to ExtOldFileType.InsidePolygon
|
|
@arthurvd I'm trying to run the converter script through the command line but it's failing with the error message below. |
Makes serialized model files lean by only serializing the field values that have been explicitly set. Only implemented for IniModel (and IniBasedModel).
On the longer term, can we get rid of `LegacyFMModel` (which only exists to support `[Model]` in MDU files), and have this backwards compatibility implemented in a less ad-hoc way?
Misc cleanup + new cmdline args: * --postfix / -p : Append POSTFIX to the output filenames (before the extension). * --[no]backup : Create a backup of each file that will be overwritten.
|
feat: initial condition converter from old external forcing file
* test initializing the `ExtOldModel` * remove unused imported modules * remove unused imported modules * reformat the test_ext_old_to_new.py and test the printed messages * reformat the test_ext_old_to_new.py and test the printed messages * add `InitialCondInterpolationMethod` class and test * add `InitialCondFileType` class and test * fix initial condition tests * create `test_meteo_forcing_file_type` to test `MeteoForcingFileType` * change tests to check for the parameter values not the parameter name * test `MeteoInterpolationMethod` in the `test_meteo_interpolation_methods` * reformat and test the Meteo class * clean tests that were moved to the TestMeteo test class * move `test_missing_required_fields` to the separate `TestMeteo` class * solve the compare floating point values * rename the old `TestMeteo` to `TestExtModel` * reformat the `TestExtModel` * add the cases for time_series/boundary condition files as forcing files to the `TestMeteo` * correct field names in the `InitialCondition` class * test the `InitialConditions` class for all possible behaviours * fix issue in assigning mutable object (list) as a default * test `construct_filemodel_new_or_existing` function * reformat the test_ext_old_to_new.py file to use fixtures instead of the tests.utils module * import the used function directly at the top of the file do not use "module.function" * name test files by their respective modules not inner functions or classes * reformat test file * move check values to the conftest file * test the `_read_ext_old_data` function * test the `InitialConditionConverter` class * reformat test * move the initial condition fields tests from the test_ext.py to the test_inifield * remove duplicated initial condition fields classes * autoformat: isort & black * move the InitialCondition tests to the tests_inifield.py * fix use mutable as default value * create and test `InitialConditionConverter` * autoformat: isort & black * user snake case parameter names * integrate the Initial_Condition converter to the `ext_old_to_new` * autoformat: isort & black * delete test results * fix test error * clean * autoformat: isort & black * remove ignored tests * use default_factory=list instead of [] to prevent using mutable objects as default value * use fixtures instead of variables declared in the `tests.utils` module * convert relative imports to absolute imports * exclude python=3.12.5 bcause of black warning about memory safety issue. * create separate group for the docs dependencies * create `ExternalForcingConverter` class to include all converter functionality * create class method for the `ExternalForcingConverter` from the old external forcing `ExtOldModel` * autoformat: isort & black * convert the `ext_old_to_new` function into a method called `update` in the `ExternalForcingConverter` class * first step to move out the `construct_filemodel_new_or_existing` calling from the `update` method * create setter and getter properties for each model, and make abstract version of the `update` method, separate the `save` functionality * reformat the converter tests * separate tests for the update, save, and add default paths to the models in the constructor method * fix the error of using mutables as a default value. * create separate test class for the update method in the converter * test files for the update meteo test * rename the meteo_converter into converters and merge the boundary coverter to it * move the initial_condition_converter to the converters module * move the initial_condition_converter to the converters module * autoformat: isort & black * add the `BoundaryConditionConverter` to the converter_factory.py * create a `converters.BoundaryConditionConverter` class * fix using mutables as default value * fix using mutables as default value * use the getattr to fetch attributes from the object * create subfolders in the test directory * get rid of relative paths * add init file to the polyline test dir * add tests for different polyline cases (basic case, and with label) * add tests for different polyline cases (basic case, and with label) * split the test_ext.py to a separate testing modules * fix error in Boundary object instantiation in the `BoundaryConditionConverter` * test the `ext.models.boundary` with an existing polyline * test the `ExternalForcingConverter.update` with only boundary condition data * replace float point comparison with np.isclose * replace float point comparison with np.isclose * remove unused imported function * add try, except clause to avoid PermissionError in Windows * update MetroForcingFileType class with updates types * silence dimr and serializer tests * autoformat: isort & black * fix error in using mutables as default value * remove the `initialsalinitytopuse` quantity from the `ExtOldInitialConditionQuantity` class. * add missing initial condition quantities and add `__missing__` method to accept tracer quantities * adjust the `tracer` quantity to `initialtracer` * add list of old initial conditions quantities that comes with a suffix and test them * update docstring * remove duplicate `InitialConditions` class (duplicate if `InitialField`) * fix floating point comparison * fix floating point comparison * remove un-used parameter `postfix and fix sonar warnings` * remove the `locationtype` and convert the value of the extrapolation to yes/no from 1/0 * the `ExternalForcingConverter` class can be instantiated by `ExtOldModel` 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 * rename boundary polylines files to `boundary-polyline-` prefix * update converter command line * return the correct return type hint --------- Co-authored-by: MAfarrag <[email protected]>
* test initializing the `ExtOldModel` * remove unused imported modules * remove unused imported modules * reformat the test_ext_old_to_new.py and test the printed messages * reformat the test_ext_old_to_new.py and test the printed messages * add `InitialCondInterpolationMethod` class and test * add `InitialCondFileType` class and test * fix initial condition tests * create `test_meteo_forcing_file_type` to test `MeteoForcingFileType` * change tests to check for the parameter values not the parameter name * test `MeteoInterpolationMethod` in the `test_meteo_interpolation_methods` * reformat and test the Meteo class * clean tests that were moved to the TestMeteo test class * move `test_missing_required_fields` to the separate `TestMeteo` class * solve the compare floating point values * rename the old `TestMeteo` to `TestExtModel` * reformat the `TestExtModel` * add the cases for time_series/boundary condition files as forcing files to the `TestMeteo` * correct field names in the `InitialCondition` class * test the `InitialConditions` class for all possible behaviours * fix issue in assigning mutable object (list) as a default * test `construct_filemodel_new_or_existing` function * reformat the test_ext_old_to_new.py file to use fixtures instead of the tests.utils module * import the used function directly at the top of the file do not use "module.function" * name test files by their respective modules not inner functions or classes * reformat test file * move check values to the conftest file * test the `_read_ext_old_data` function * test the `InitialConditionConverter` class * reformat test * move the initial condition fields tests from the test_ext.py to the test_inifield * remove duplicated initial condition fields classes * autoformat: isort & black * move the InitialCondition tests to the tests_inifield.py * fix use mutable as default value * create and test `InitialConditionConverter` * autoformat: isort & black * user snake case parameter names * integrate the Initial_Condition converter to the `ext_old_to_new` * autoformat: isort & black * delete test results * fix test error * clean * autoformat: isort & black * remove ignored tests * use default_factory=list instead of [] to prevent using mutable objects as default value * use fixtures instead of variables declared in the `tests.utils` module * convert relative imports to absolute imports * exclude python=3.12.5 bcause of black warning about memory safety issue. * create separate group for the docs dependencies * create `ExternalForcingConverter` class to include all converter functionality * create class method for the `ExternalForcingConverter` from the old external forcing `ExtOldModel` * autoformat: isort & black * convert the `ext_old_to_new` function into a method called `update` in the `ExternalForcingConverter` class * first step to move out the `construct_filemodel_new_or_existing` calling from the `update` method * create setter and getter properties for each model, and make abstract version of the `update` method, separate the `save` functionality * reformat the converter tests * separate tests for the update, save, and add default paths to the models in the constructor method * fix the error of using mutables as a default value. * create separate test class for the update method in the converter * test files for the update meteo test * rename the meteo_converter into converters and merge the boundary coverter to it * move the initial_condition_converter to the converters module * move the initial_condition_converter to the converters module * autoformat: isort & black * add the `BoundaryConditionConverter` to the converter_factory.py * create a `converters.BoundaryConditionConverter` class * fix using mutables as default value * fix using mutables as default value * use the getattr to fetch attributes from the object * create subfolders in the test directory * get rid of relative paths * add init file to the polyline test dir * add tests for different polyline cases (basic case, and with label) * add tests for different polyline cases (basic case, and with label) * split the test_ext.py to a separate testing modules * fix error in Boundary object instantiation in the `BoundaryConditionConverter` * test the `ext.models.boundary` with an existing polyline * test the `ExternalForcingConverter.update` with only boundary condition data * replace float point comparison with np.isclose * replace float point comparison with np.isclose * remove unused imported function * add a pull request template * correct test folder name from extold to ext * optimize the `ext.models.Boundary.forcing` property to have one return statement at the end of the property. * test the `ExtOldParametersQuantity` class * correct assert statement is `test_ext_old_initial_condition_quantity` test * move the TestModels.TestLateral from thr test_ext.py to a separate test file test_laterals.py * correct the test file description * reformat the test_boundary.py file * clean the test file from unused imported functions * remove unused class `ext.models.InitialConditions` * add reference to the initial condition documentation in the `InitialField` class * add `ParametersConverter` class and test * merge the `BaseConverter` class to the `converters` module * test `MeteoConverter` * merge `converter_factory` module to the `converters` module * merge the `enum_converters` module to `utils` * merge the `__contains__` to the `ConverterFactory` class * abstract duplicate lines in the parameter and initial condition converters * add try, except clause to avoid PermissionError in Windows * update MetroForcingFileType class with updates types * silence dimr and serializer tests * autoformat: isort & black * fix error in using mutables as default value * remove the `initialsalinitytopuse` quantity from the `ExtOldInitialConditionQuantity` class. * add missing initial condition quantities and add `__missing__` method to accept tracer quantities * adjust the `tracer` quantity to `initialtracer` * add list of old initial conditions quantities that comes with a suffix and test them * update docstring * remove duplicate `InitialConditions` class (duplicate if `InitialField`) * fix floating point comparison * fix floating point comparison * remove un-used parameter `postfix and fix sonar warnings` * remove the `locationtype` and convert the value of the extrapolation to yes/no from 1/0 * the `ExternalForcingConverter` class can be instantiated by `ExtOldModel` 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 * rename boundary polylines files to `boundary-polyline-` prefix * update converter command line * update references to the `Inifield` class from the documentation * rename the `Advectiontype` to `Advectiontype` in the `ExtOldParametersQuantity` class * rename the `create_convert_inputs` to a descriptive name `create_initial_cond_and_parameter_input_dict` and add docstring * update the pull request template * correct the docstring * replace the string meteo interpolation method by the class method itself * Fix typo in converters.py * Fix docstring for ParameterField in converters.py * fix enum values in test asserts for meteo converter * check docstring * remove duplicated test file because of wrong renaming --------- Co-authored-by: MAfarrag <[email protected]> Co-authored-by: Arthur van Dam <[email protected]>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #621