prof_to_cross utility#488
Conversation
Done: * commandline argparse * callable for either mdu of prof* files * converts profloc Todo: * convert profdef * convert profdefxyz
* also added APIdocs * also added first version of end-user usage docs.
…HYDROLIB-core into feat/487_prof_to_cross
|
Kudos, SonarCloud Quality Gate passed!
|
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
Should something be added to this file?
| @@ -0,0 +1,545 @@ | |||
| import argparse | |||
There was a problem hiding this comment.
Do we want to put these file in the tools folder or in a sub folder in the tools folder? e.g. tools/converters
There was a problem hiding this comment.
would it be an idea to splitup this file into smaller files?
There was a problem hiding this comment.
The conclusion of last week's discussion was: begin with all tools directly inside the tools directory.
But now that this stis here, let's double check tomorrow (also taking your question below into account).
| @@ -0,0 +1,46 @@ | |||
| import os | |||
There was a problem hiding this comment.
Should the cmd line input (main) also be tested?
There was a problem hiding this comment.
these tests only cover the prof_to_cross & prof_to_cross_from_mdu "public" methods, do we also need to test the other "public" methods?
| if _proftype >= 2: | ||
| _proftype = math.floor(_proftype / 2.0) * 2 | ||
|
|
||
| if _proftype == 1: # Pipe |
There was a problem hiding this comment.
Maybe you can use a dictionary to do this?
_crstype_map = {
1: "circle",
2: "rectangle",
4: "yz",
6: "yz",
100: "yz",
200: "xyz",
}
if _proftype in _crstype_map:
crstype = _crstype_map[_proftype]
else:
raise ValueError(f"Invalid legacy profile type given: TYPE={proftype}")
There was a problem hiding this comment.
I thought about this! But then I thought: YAGNI.
I'll think about it
| str: Equivalent frictionType string value for use in a crsdef.ini. | ||
| """ | ||
|
|
||
| if frctp == 0: |
There was a problem hiding this comment.
Maybe the same refactor as described by _proftype_to_crsdeftype?
|
|
||
| (proftype, crstype) = _proftype_to_crsdeftype(profdef.get("TYPE")) | ||
|
|
||
| crsdata = {} |
There was a problem hiding this comment.
Maybe the same refactor as described by _proftype_to_crsdeftype?
You could also move this to a seperate method.
|
|
||
| crsdata["id"] = f"PROFNR{profdef['PROFNR']}" | ||
| crsdata["type"] = crstype | ||
| if crstype == "circle": |
There was a problem hiding this comment.
Maybe the same refactor as described by _proftype_to_crsdeftype?
You could also move this to a seperate method.
| crslocfile: PathOrStr = None, | ||
| crsdeffile: PathOrStr = None, | ||
| ): | ||
| """The actual converter function for converting legacy profile files |
There was a problem hiding this comment.
does "actual" add value to this description?
There was a problem hiding this comment.
how do we want to make a distinction between the wrapper and this method towards the users in the method description?
There was a problem hiding this comment.
Is the addition in the wrapper function [..], for files listed in an MDU file. not sufficiently clear?
| crslocfile: PathOrStr = None, | ||
| crsdeffile: PathOrStr = None, | ||
| ): | ||
| """Wrapper converter function for converting legacy profile files |
There was a problem hiding this comment.
Do we need to state that it is a wrapper, is that information usefull for the user?
There was a problem hiding this comment.
"Wrapper" can go for me, but it's to stress that this has MDU as input, but for the rest is the same as the actual converter function. Note that "user" is a "developer" here.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the prof_to_cross utility for converting legacy D-Flow FM profile files to cross section files. It adds new tests, documentation, and a supporting context manager in the core utilities to facilitate the tool's operation.
- Added unit tests for processing both profile files and MDU files.
- Updated documentation and navigation to incorporate prof_to_cross.
- Introduced a new working_directory context manager in the core utils.
Reviewed Changes
Copilot reviewed 7 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/tools/test_prof_to_cross.py | New tests for validating prof_to_cross functionality |
| mkdocs.yml | Updated navigation to include prof_to_cross documentation |
| hydrolib/tools/init .py | Added an (empty) module initialization file for tools |
| hydrolib/core/utils.py | Added working_directory context manager to manage CWD changes |
| docs/tools/tools.md | Added an overview of built-in tools including prof_to_cross |
| docs/tools/prof_to_cross.md | Added detailed documentation for prof_to_cross |
Files not reviewed (10)
- tests/data/input/prof_to_cross/profdef.txt: Language not supported
- tests/data/input/prof_to_cross/profdef_withxyz.txt: Language not supported
- tests/data/input/prof_to_cross/profdefxyz.pliz: Language not supported
- tests/data/input/prof_to_cross/profloc.xyz: Language not supported
- tests/data/input/prof_to_cross/profloc_withxyz.xyz: Language not supported
- tests/data/input/prof_to_cross/tree1d2d.mdu: Language not supported
- tests/data/reference/prof_to_cross/crsdef.ini: Language not supported
- tests/data/reference/prof_to_cross/crsdef_withxyz.ini: Language not supported
- tests/data/reference/prof_to_cross/crsloc.ini: Language not supported
- tests/data/reference/prof_to_cross/crsloc_withxyz.ini: Language not supported
Comments suppressed due to low confidence (2)
hydrolib/core/utils.py:186
- The 'Path' identifier is used without being imported. Consider adding 'from pathlib import Path' at the top of the file.
prev_cwd = Path.cwd()
Fixes #487