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

Newext file missing validation #533

Open
veenstrajelmer opened this issue Apr 13, 2023 · 1 comment
Open

Newext file missing validation #533

veenstrajelmer opened this issue Apr 13, 2023 · 1 comment
Labels
domain: validation type: question Further information is requested

Comments

@veenstrajelmer
Copy link
Collaborator

veenstrajelmer commented Apr 13, 2023

Describe the bug
When generating a Boundary and adding it to an ExtModel, the user can provide several incorrect inputs that are not validated against. An overview is in #TODO-comments in the code in this issue.

Some aditional waterlevelbnd specific validation:

  • Correction April 26: NO validation is necessary for the ORDER of salinitybnd (and other constituent bnd) with respect to the flow boundaries on the same location. @arthurvd has confirmed that salinitybnd can come before e.g. waterlevelbnd without problems.
  • Correction April 26: No validation is yet needed for a keyword operand, because that is not supported yet in the kernel. Only when it is added there, we will update hydrolib (under another future issue).
    I noticed it is not possible to supply an operand keyword to the Boundary object. When supplying two consequetive waterlevelbnds linking to the same physical plifile, the forcings are added (+). When supplying e.g. in the order of waterlevelbnd/salinitybnd/waterlevelbnd, the second waterlevelbnd overwrites (O) the first one. This also happens if the plifile which they link to is a different file. Relevant issue: https://issuetracker.deltares.nl/browse/UNST-5320. Possible to validate all of this in hydrolib-core, or to improve the FM kernel code. Currently, providing two waterlevelbnd's results in ** WARNING: Boundary link 00001662 already claimed [ -68.0750, 12.6625] when running the model.
  • Enhancement requested: validate the value for quantity: maintain list of supported quantities (see https://content.oss.deltares.nl/delft3dfm1d2d/D-Flow_FM_User_Manual_1D2D.pdf#table.C.5 )
  • Enhancement: for locationfile, add PolyFile in a union to the current DiskOnlyFileModel
  • Needs refinement: forcingfile=forcing_object, #TODO: not validated if this is an existing file (when not saving forcing_object, the "forcingFile" property in the extfile will be missing and therefore this file will be invalid)
  • Enhancement: switch off allow_extra for Boundary.

To Reproduce

import hydrolib.core.dflowfm as hcdfm

file_pli = 'test_model.pli'
with open(file_pli,'w') as f:
    f.write("""name
                1    2
                1.0    2.0
                3.0    4.0
                """)

ext_object = hcdfm.ExtModel()
forcing_object = hcdfm.ForcingModel()
forcing_object.save('test_model.bc')

#TODO: support for relative paths?
#generate boundary object for the ext file (quantity, pli-filename, bc-filename)
boundary_object = hcdfm.Boundary(quantity='nonexistingbnd', #TODO: not validated if it is a valid quantity
                                 locationfile=file_pli, #TODO: not validated if this is an existing file, not validated if it is a valid plifile (2x2 block while it says 1x2)
                                 forcingfile=forcing_object, #TODO: not validated if this is an existing file (when not saving forcing_object, the "forcingFile" property in the extfile will be missing and therefore this file will be invalid)
                                 nonexistingkey='value') #TODO: not validated if this key exists

ext_object.boundary.append(boundary_object)
ext_object.save('test_model.ext')

Expected behavior
More consise validation for all comments in example code and the waterlevelbnd specific things.

Version info (please complete the following information):

  • OS: [e.g. Windows]
  • Version [e.g. 0.1.5]
@arthurvd
Copy link
Member

  • A different order therefore impacts the model results, so it would be good to validate this in hydrolib-core.

I understand your wish, but recommend to not implement it in hydrolib-core. Motivation:

  • The precise check in the kernel is done per open boundary flowlink, so not per boundary polyline. Nothing guarantees that the boundary polyline for the transported matter (salinity, or tracer, etc.) is equal to the polyline for the flow boundary: not in filepath, and not even in actual polyline points. The kernel does not and will not require it: you might want to have a small localized tracer boundary polyline on top of a longer open flow boundary polyline.
    hydrolib-core does not know about the grid-snapped boundary polylines, so cannot do the same validation on flow links.
  • I'm doubting whether there's then any value to still have a "lightweight check" in hydrolib-core that would only check on order if the location_file paths are the same. What do you think, @veenstrajelmer ?

@arthurvd arthurvd added this to To do in HYDROLIB-core via automation Apr 21, 2023
@veenstrajelmer veenstrajelmer changed the title Newext file missing validation and missing operand keyword Newext file missing validation Apr 26, 2023
@priscavdsluis priscavdsluis added type: question Further information is requested domain: validation and removed help wanted labels Apr 9, 2024
@rhutten rhutten removed this from To do in HYDROLIB-core Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: validation type: question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants