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

Polyfile parser does not parse 2D boundary type of pli file correctly. #467

Closed
xldeltares opened this issue Mar 14, 2023 · 7 comments · Fixed by #482
Closed

Polyfile parser does not parse 2D boundary type of pli file correctly. #467

xldeltares opened this issue Mar 14, 2023 · 7 comments · Fixed by #482
Assignees
Labels
type: bug Something isn't working
Milestone

Comments

@xldeltares
Copy link
Collaborator

xldeltares commented Mar 14, 2023

Describe the bug
Polyfile parser does not parse 2D boundary type of pli file correctly.
See example file: Boundary01.pli.txt

To Reproduce
Steps to reproduce the behavior:

from pathlib import Path
from hydrolib.core.dflowfm.polyfile.parser import read_polyfile
fn = Path(r'Boundary01.pli')
read_polyfile(filename)
{'has_z_values': False, 'objects': []}

See the empty objects, while the 'Boundary01.pli' is not empty:
"
Boundary01
3 2
6.625216613133794E+005 1.524474171041823E+006 Boundary01_0001
6.625216613133794E+005 1.524910832891534E+006 Boundary01_0002
6.625216613133794E+005 1.525410832891534E+006 Boundary01_0003
"

Expected behavior
Able to parse this file for the 2D boundary.

Suggested solution
(by @arthurvd )
The third column of support point labels should be ignored by the parser (but NOT lead to an error). The header definition is leading: if it specifies 2 columns, then only read 2 values from each line, and silently ignore anything that's left at the end.

My motivation for this solution is: the Delft3D FM Suite currently exports with a support point label on each line, and we want to support all models files produced by our official release.
However, there is no need to parse and store those labels, because they are in fact ignored right now by the kernel, because frontend and backend are not using the same format currently (even though they are present in the .pli files)
This first need to be fixed in frontend+backend. Only then we will consider full support in hydrolib-core.

Version info (please complete the following information):

  • OS: [e.g. Windows]
  • Version [e.g. 0.4.1]
@veenstrajelmer
Copy link
Collaborator

veenstrajelmer commented Mar 15, 2023

Hi @xldeltares, minor comment, from hydrolib.core.dflowfm.polyfile.parser import read_polyfile is only meant for internal hydrolib-core usage, so not to be called by users. Could you test the loading of the polyfile with this code?

from pathlib import Path
import hydrolib.core.dflowfm as hcdfm
file_pli = Path(r'Boundary01.pli')
polyfile_object = hcdfm.PolyFile(file_pli)

If you got your code from some documentation, please let me/@arthurvd know so it can be updated.

If you use this code in the next release of hydrolib-core, you will get an error, so it is at least clear that the polyfile is not being read.

ValueError: Invalid formatted plifile, Expected a valid next point at line 2.
Invalid block 0:5
File: Boundary01.pli

@xldeltares
Copy link
Collaborator Author

Thanks, @arthurvd and @veenstrajelmer for the explanation! Now I understand why the supported point labels are not supported by hydrolib-core. And I agree that the fix should first be the front- and back-end.

Since the pli file without supported point labels can indeed be read back to the GUI, I think we are good off with reading/writing pli files without supported point labels in hydromt-delft3dfm.
That means we won't support updating a model that was exported from GUI, which was acceptable.
If this behavior would change, we will seek dfm-tools for preprocessing the pli file.

@xldeltares
Copy link
Collaborator Author

If you got your code from some documentation, please let me/@arthurvd know so it can be updated.

Thanks! I did not get the code from the documentation, it was a small test of "Why my PolyFile object is empty" :)

@arthurvd
Copy link
Member

Hi @xldeltares, I will re-open this issue, because I think hydrolib-core should support reading of boundary polylines currently produced by the FM Suite (both kernel and GUI support reading of pli point labels, only they are not used in the actual calculation).

@arthurvd
Copy link
Member

Hi @xldeltares, I know that you are currently modifying boundary pli files, removing the last column with point labels.
Yet, we have fixed the support for this in hydrolib-core.
Could you pleas make some time to test the new support on your (unmodified) pli files?
The code is in:
#482
Thanks!

@veenstrajelmer
Copy link
Collaborator

veenstrajelmer commented Mar 31, 2023

@arthurvd, I see @xldeltares is out of office and I had a testcase ready so I tested it. Reading a polyfile with labels now works and is correctly parsed.

arthurvd added a commit that referenced this issue Mar 31, 2023
@xldeltares
Copy link
Collaborator Author

@veenstrajelmer Thanks for picking this up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants