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: external forcings converter #647

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

arthurvd
Copy link
Member

@arthurvd arthurvd commented Jun 6, 2024

Closes #621

* basic program structure
* processing of a single [Meteo] block as first example code
* testcase c081 with multiple combined wind files.
Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@MRVermeulenDeltares MRVermeulenDeltares Jun 13, 2024

Choose a reason for hiding this comment

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

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

image

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?

FlorisBuwaldaDeltares and others added 8 commits July 4, 2024 11:25
…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
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
5.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@RStolkerDeltares
Copy link

RStolkerDeltares commented Sep 13, 2024

@arthurvd I'm trying to run the converter script through the command line but it's failing with the error message below.
I did install the poetry environment as described in the docs.

poetry run python main_converter.py

Traceback (most recent call last):
  File "main_converter.py", line 35, in <module>
    from .converter_factory import ConverterFactory
ImportError: attempted relative import with no known parent package

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.
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
5.2% Duplication on New Code (required ≤ 3%)
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@MAfarrag MAfarrag added type: feature Brand new functionality priority: high labels Dec 5, 2024
feat: initial condition converter from old external forcing file
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

Converter for old external forcings file
6 participants