-
Notifications
You must be signed in to change notification settings - Fork 4
UNST-9261: Convert old longculverts on-the-fly before partitioning #413
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
base: main
Are you sure you want to change the base?
UNST-9261: Convert old longculverts on-the-fly before partitioning #413
Conversation
…ing does not seem correct without writing those files.
…1. Increase absolute tolerance from 0.001 to 0.005. Maybe it's too much?
FlorisBuwaldaDeltares
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! One small question
| mkdir -p "$(dirname $TESTBENCH_ARTIFACTS)" | ||
| if [[ ! -d "$TESTBENCH_ARTIFACTS" ]]; then | ||
| ln -s -T "$(realpath build_fm-suite_release/install/)" "$TESTBENCH_ARTIFACTS" | ||
| ln -s -T "$(realpath -m build_fm-suite_release/install/)" "$TESTBENCH_ARTIFACTS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh this seems like an unrelated change? what does this fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it is unrelated. This should go in a separate issue for improving devcontainer usability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove it from this PR
..._gpl/dflowfm/packages/dflowfm_kernel/src/dflowfm_utils/rest_f90/generatepartitionmdufile.f90
Show resolved
Hide resolved
| private | ||
|
|
||
| public :: generatepartitionmdufile | ||
| public :: generatePartitionMDUFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this routine not have snake_case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer snake_case as well, but I didn't change the name. I'll change it to snake_case
| <checks> | ||
| <file name="DFM_OUTPUT_tunnel/tunnel_0000_his.nc" type="NETCDF"> | ||
| <parameters> | ||
| <parameter name="longculvert_discharge" toleranceAbsolute="0.005" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for reviewers: The tolerance here is quite high. I had to set it to 0.005 so the testcase would pass on both linux and windows with the same references. Is it ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the long culvert has a bit of instabilities during initialization. For this reason i'm not worrying about those small discharge differences, but it might be something to revisit later.
What was done
A model with old style long culverts should automatically be converted to a model with new style long culverts (For now if the setting
convertLongCulvertsis enabled)When running
dflowfmwith the--partitionflag on a model with old longculverts, the converted longculverts should end up in the partitioned netfile, and the partitioned mdu file should contain references to the converted structures.ini and the converted cross sections file.Evidence of the work done
Tests
<add testcase numbers if applicable, Issue number>
Documentation
<add description of changes if applicable, Issue number>
Issue link