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

Inconsistent keyword usage in closed property of rectangular profile #545

Open
Kreef opened this issue May 16, 2023 · 5 comments
Open

Inconsistent keyword usage in closed property of rectangular profile #545

Kreef opened this issue May 16, 2023 · 5 comments
Labels
priority: high type: bug Something isn't working type: compatibility Changes needed to be compatible with the computational core

Comments

@Kreef
Copy link

Kreef commented May 16, 2023

Describe the bug
The closed keyword in HYDROLIB-core/hydrolib/core/dflowfm/crosssection/models/RectangleCrsDef defaults to True, but D-HDYRO (v23.01) expects "yes" and "no"; as described in the user manual (Table C.24)

Expected behavior
write yes or no to crsdef.ini

replacing: closed: str = Field(True)
by: closed: str = Field("yes")
fixed it for me

@priscavdsluis priscavdsluis added type: bug Something isn't working type: compatibility Changes needed to be compatible with the computational core priority: high labels Apr 9, 2024
@rhutten rhutten added this to To do in HYDROLIB-core via automation Apr 10, 2024
@tim-vd-aardweg
Copy link
Contributor

@rhutten I was told that if the kernel expects a boolean for a keyword they accept all values such as: 1, y, yes, t, true, j, ja and several more. Given that the kernel can handle all these values, do you think we should still update how the closed keyword is written to the file?

In HYDROLIB-core we basically always write booleans as 0 or 1.

@veenstrajelmer
Copy link
Collaborator

@tim-vd-aardweg I agree to stick with a boolean for consistency reasons. @Kreef did you encounter a crash in a D-HYDRO run or did you merely point this out because of an inconsistency between the manual and HYDROLIB-core? I think we should in that case update the D-HYDRO manual.

@Kreef
Copy link
Author

Kreef commented May 8, 2024

@veenstrajelmer I cannot verify the problem anymore as I am not working with D-HYDRO anymore, but this issue was not solely pointing out an inconsistency. I believe it either caused the affected profile to not be loaded in the GUI, or the DIMR set to not being loaded in the GUI.

@arthurvd
Copy link
Member

arthurvd commented May 8, 2024

Regarding consistency: I've decided do try and change all boolean settings in D-Flow FM to print true and false in mdu/dia/ini files, and will include the frontend development team to also try and make that change (ref: UNST-7950).
Backwards compatibility in kernel: old yes/no/1/0 values will still be supported for a while, but hydrolib-core can stop supporting old values in an upcoming release.

@arthurvd
Copy link
Member

arthurvd commented May 8, 2024

@tim-vd-aardweg : I think remaining work for hydrolib-core is to update the matching fields in the various Comments classes, for example:

        closed: Optional[str] = Field("no: Open channel, yes: Closed channel.")
...
        usevolumetables: Optional[str] = Field(
            "Use volume tables for 1D grid cells (1: yes, 0 = no).",
...

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 type: compatibility Changes needed to be compatible with the computational core
Projects
HYDROLIB-core
  
To do
Development

No branches or pull requests

5 participants