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

Extfilenew not found with pathstyle unix #516

Open
veenstrajelmer opened this issue Apr 6, 2023 · 5 comments
Open

Extfilenew not found with pathstyle unix #516

veenstrajelmer opened this issue Apr 6, 2023 · 5 comments
Assignees
Labels
priority: high type: bug Something isn't working

Comments

@veenstrajelmer
Copy link
Collaborator

veenstrajelmer commented Apr 6, 2023

Describe the bug
When using path_style='unix' on Windows, the extfile cannot be added to the mdu since it cannot be found:

ValidationError: 1 validation error for ExternalForcing
extforcefilenew -> test_model_bc.ext -> boundary -> 0 -> forcingFile
  File: `C:\c\test_model\test_model.bc` not found, skipped parsing. (type=value_error)

It is searching for a windows style path (which is good), but the c-drive is added twice. In a less portable test, it was searching for the unix-style path (which could of course not be found).

Also, I deliberately used different names in the pli/bc files of the testcase below. If it does not cost a lot of overhead, it might be nice to validate if the name combinations are valid (low priority).

To Reproduce
This is a MWE to reproduce:

import os
import hydrolib.core.dflowfm as hcdfm

model_name = 'test_model'
dir_output = r'c:\test_model'

path_style = 'unix' # windows / unix

#create dummy files
if not os.path.exists(dir_output):
    os.mkdir(dir_output)
file_pli = os.path.join(dir_output,f'{model_name}.pli')
with open(file_pli,'w') as f:
    f.write("""name
                1    2
                1.0    2.0
                3.0    4.0
                """)

file_bc = os.path.join(dir_output,f'{model_name}.bc')
with open(file_bc,'w') as f:
    f.write("""[Forcing]
               name              = right01_0001
               function          = timeseries
               timeInterpolation = linear
               quantity          = time
               unit              = minutes since 2001-01-01
               quantity          = waterlevelbnd
               unit              = m
                  0.000000 2.50
               1440.000000 2.50
                """)

ForcingModel_object = hcdfm.ForcingModel(file_bc)
boundary_object = hcdfm.Boundary(quantity='waterlevelbnd',
                                 locationfile=file_pli, #TODO: does not check if name of forcing is found in plifile
                                 forcingfile=ForcingModel_object)

mdu_file = os.path.join(dir_output, f'{model_name}.mdu')
mdu = hcdfm.FMModel()
ext_file_new = os.path.join(dir_output, f'{model_name}_bc.ext')
ext_bnd = hcdfm.ExtModel()

ext_bnd.boundary.append(boundary_object)
ext_bnd.save(ext_file_new,path_style=path_style)

mdu.external_forcing.extforcefilenew = ext_file_new #TODO: file not found if unix path_style

Expected behavior
The extfile is found, independent on the platform-style. Also, the plifile is incorrect (2x2, but says 1x2), but this gives no validation error.
There is a workaround to supply the object instead of the path (so mdu.external_forcing.extforcefilenew = ext_new), which works. However, when providing a mdu.geometry.network, mdu.geometry.fixedweirfile, mdu.external_forcing.extforcefile or any other reference to a file, it is possible to supply a file path. Therefore, I would expect that this would also work for mdu.external_forcing.extforcefilenew.

Version info (please complete the following information):

  • OS: Windows
  • Version main branch
@arthurvd
Copy link
Member

@priscavdsluis : the duplicate C:\C\ may be something small that needs looking into, but in general this is not supported until issue #480 has been resolved.
cc @veenstrajelmer

@veenstrajelmer
Copy link
Collaborator Author

Would be great if the duplicate is indeed resolved, more complex conversion can indeed be resolved in that separate issue. The validation does work for all other files (net, oldext, etc), so would be great if the validation of newext also works like they do

@MRVermeulenDeltares MRVermeulenDeltares self-assigned this Apr 14, 2023
@priscavdsluis
Copy link
Contributor

@MRVermeulenDeltares, @veenstrajelmer:
From the example:

mdu.external_forcing.extforcefilenew = ext_file_new

Should be replaced with:

mdu.external_forcing.extforcefilenew = ext_bnd

This is because extforcefilenew should be an ExtModel and not a path.

@priscavdsluis priscavdsluis added type: bug Something isn't working priority: high labels Apr 9, 2024
@tim-vd-aardweg
Copy link
Contributor

Should we fix #480 first? That issue is not currently on the board.
Shall we create a separate issue for the incorrect pli-file?
@veenstrajelmer & @rhutten

@tim-vd-aardweg
Copy link
Contributor

Assigned the issue to @rhutten, She will look into it to determine if we should fix #480 first.

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

No branches or pull requests

6 participants