Skip to content

Add most recent version of LWKM setup scripts#438

Open
jvleeuwen00 wants to merge 28 commits into
mainfrom
lwkm_sprint
Open

Add most recent version of LWKM setup scripts#438
jvleeuwen00 wants to merge 28 commits into
mainfrom
lwkm_sprint

Conversation

@jvleeuwen00

Copy link
Copy Markdown
Collaborator

No description provided.

@jvleeuwen00 jvleeuwen00 marked this pull request as draft November 24, 2025 15:24
@jvleeuwen00 jvleeuwen00 requested a review from gpijcke November 26, 2025 10:02
@gpijcke gpijcke marked this pull request as ready for review November 26, 2025 10:06

@gpijcke gpijcke left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • move all paths to the top of the script under a header "user input" or so. This to avoid user needs to scroll through the file to find where input is required.
  • move files with data to "the good cloud". Now they are taken from p-drive
  • division of emissions by 2 (because there are 2 ribasim basins per area) is hardcoded. please change this into a variable input at the top of the script, e.g. frac_doorvoerend = 0.5 and frac_bergend = 0.5 so that it is transparent. please also add a simple check that frac_doorvoerend + frac_bergend = 1 and issue an error if that is not the case.
  • graph "sum of fraction per nodeid": this is a check on the input I think? Good that it is there but would be nice to then have additional feedback to the user when the graphs shows the input has been correctly processed and when not
  • variable "ER_data_EMK_GAF90_fltr_sum_piv" has a column which I think does not get used. I think it functions as an index. Can we rename the column as such or can we remove it all together?
  • This line "emissies_totaal = emissies_totaal.rename(columns={"Stof": "VariableId"})" (line 495)seems superfluous. There is no column with name "Stof" anymore at this point.
  • The script should generate another file, listing all loads. This file should look as follows:
    XXX ; number of loads
    ; SegmentID Load-name Comment Load-type
    1 'load1' ' ' ' ' ; load1
    etc.

where XXX is the total number of loads (integer number)
The file should have the mapping between delwaq segment (first column) with the ribasim basin id (column 2) which is currently used to prepare the ER loads file (B6_loads.inc) for Delwaq. This ensures that the ER loads enter at the correct Delwaq segment.

See screenshot:
image

  • instead of "LINEAR", please use "BLOCK" as interpolation method. The ER values are valid for a one year period. The "BLOCK" interpolation ensures that for the full year these values are used.
  • Please add logging at certain moment in the code so that the user gets feedback on the progress. Suggestions: "B6_loads.inc" written "c:\xxx\yyy", "Mapping loads GAF units to Ribasim basins", etc.
  • add "usefor" statements to each of the loads so that the totn and totp get split into inorganic and organic fractions:
    ITEM '200001' CONCENTRATION
    USEFOR 'NO3' 'TotN' * 0.8
    USEFOR 'NH4' 'TotN' * 0.1
    USEFOR 'OON' 'TotN' * 0.1
    TIME BLOCK
    DATA 'TotN'
    '2010/01/01-00:00:00' 0.031083
    '2011/01/01-00:00:00' 0.031355
    '2012/01/01-00:00:00' 0.031188
    '2013/01/01-00:00:00' 0.031175
    '2014/01/01-00:00:00' 0.031222
    '2015/01/01-00:00:00' 0.031092
    '2016/01/01-00:00:00' 0.030313
    '2017/01/01-00:00:00' 0.030286
    '2018/01/01-00:00:00' 0.029088
    '2019/01/01-00:00:00' 0.029489
    '2020/01/01-00:00:00' 0.028898
    '2021/01/01-00:00:00' 0.028578

ITEM '200001' CONCENTRATION
USEFOR 'PO4' 'TotP' * 0.5
USEFOR 'AAP' 'TotP' * 0.4
USEFOR 'OOP' 'TotP' * 0.1
TIME BLOCK
DATA 'TotP'
'2010/01/01-00:00:00' 0.001330
'2011/01/01-00:00:00' 0.001400
'2012/01/01-00:00:00' 0.001324
'2013/01/01-00:00:00' 0.001300
'2014/01/01-00:00:00' 0.001295
'2015/01/01-00:00:00' 0.001231
'2016/01/01-00:00:00' 0.001103
'2017/01/01-00:00:00' 0.001225
'2018/01/01-00:00:00' 0.000957
'2019/01/01-00:00:00' 0.001222
'2020/01/01-00:00:00' 0.001156
'2021/01/01-00:00:00' 0.001147

…anges to ER_GAF_fractions_func.py to account for partitioning of emissions to bergend and doorgaand basins

Also added partitioning of Ntot and Ptot into actual fractions NH4, PO4, etc. and a function to write the correct B6_loads.inc file with these fractions. This has been tested successfully.

Added combined_testing.py, a combination of setup_delwaq.py and run_parse_inspect_delwaq.py to facilitate easier testing of the entire workflow in one script..
@jvleeuwen00

jvleeuwen00 commented Dec 5, 2025

Copy link
Copy Markdown
Collaborator Author

RESPONSE TO REVIEW BY @gpijcke

In general, the script should be revised a bit so we clearly know what each step does and what the printed figures are meant to check. Right now this script creates a large amount of figures along the way, which may prevent it from running fully in one go (the amount of figures that VSCode can show is limited and the script will not continue unless you close some of these figures again in my case). This means some of the plot commands should preferably be deleted. However, at this time I do not know which ones are useful and which ones aren't.

  • move all paths to the top of the script under a header "user input" or so. This to avoid user needs to scroll through the file to find where input is required.

Moved 'Directories' block (and 'Settings') to the top of the script. ~ is this the 'pythonic' way to order your script, or is functions first the norm?

Added a block 'ensure correct working directory' as running (parts of) the setup script may set the directory to the wrong folder. This will not be necessary when this file becomes fully executable from a 'master' script

Changed the overall structure of the script to a slightly more sensible format

  • move files with data to "the good cloud". Now they are taken from p-drive

Suggestion noted for later. Probably good to download a new dataset at ER so we can document exactly how this was obtained

  • division of emissions by 2 (because there are 2 ribasim basins per area) is hardcoded. please change this into a variable input at the top of the script, e.g. frac_doorvoerend = 0.5 and frac_bergend = 0.5 so that it is transparent. please also add a simple check that frac_doorvoerend + frac_bergend = 1 and issue an error if that is not the case.

Module "ER_GAF_functions_func.py" adjusted to also return the variable meta_category of each polygon. Required joining the : "Basin / area" layer to the "Node" layer from the model geopackage

In the main script included the variables:

• frac_doorgaand = 0.5
• frac_bergend = 1 - frac_doorgaand

Which may be adjusted to liking.

What can be seen in the script is that, despite partitioning the fractions over the two main basin node types, there is still a large portion (20%) of the GAF nodes which are partitioned for more than 100%, meaning the emissions in this polygon are given to multiple basin polygons that occupy the same space (up to 200%). The primary way to fix this would be to fix the shapes of the basin polygons in the LHM so that there is no overlap. Another method could be to downscale the emissions from the GAF nodes that exceed 100% partitioning such that they arrive at exactly 100%, thereby reducing the emissions on all underlying basin nodes. It should also be noted that a major portion of the GAF nodes is not fully partitioned over the nodes, which is either the result of GAF-units partly lying outside of the LHM domain and partly because of spatial gaps in the model (as can be seen around some rivers). Future LHM improvements may solve this.

image
  • graph "sum of fraction per nodeid": this is a check on the input I think? Good that it is there but would be nice to then have additional feedback to the user when the graphs shows the input has been correctly processed and when not

See above for context, added 'should not exceed 1.0' in the title. Any ideas for further clarification are welcome.

  • variable "ER_data_EMK_GAF90_fltr_sum_piv" has a column which I think does not get used. I think it functions as an index. Can we rename the column as such or can we remove it all together?

NOT RESOLVED - requires thorough revision of the workflow in this script

  • This line "emissies_totaal = emissies_totaal.rename(columns={"Stof": "VariableId"})" (line 495)seems superfluous. There is no column with name "Stof" anymore at this point.

NOT RESOLVED - requires thorough revision of the workflow in this script

  • The script should generate another file, listing all loads. This file should look as follows:
    XXX ; number of loads
    ; SegmentID Load-name Comment Load-type
    1 'load1' ' ' ' ' ; load1
    etc.

where XXX is the total number of loads (integer number) The file should have the mapping between delwaq segment (first column) with the ribasim basin id (column 2) which is currently used to prepare the ER loads file (B6_loads.inc) for Delwaq. This ensures that the ER loads enter at the correct Delwaq segment.

As discussed, this file is currently made in the same script as where generate.py is run (and ideally should be included within generate.py itself)

  • instead of "LINEAR", please use "BLOCK" as interpolation method. The ER values are valid for a one year period. The "BLOCK" interpolation ensures that for the full year these values are used.

Replaced:
LINEAR TIME LINEAR DATA

with :
ABSOLUTE TIME BLOCK DATA

  • Please add logging at certain moment in the code so that the user gets feedback on the progress. Suggestions: "B6_loads.inc" written "c:\xxx\yyy", "Mapping loads GAF units to Ribasim basins", etc.

Some logging added

  • add "usefor" statements to each of the loads so that the totn and totp get split into inorganic and organic fractions

As discussed, we are not doing usefor but define the fractions in our python scripts beforehand. This makes the transition to automatic input file writing with generate.py easier in a later stage.

@gijsber

gijsber commented Dec 5, 2025

Copy link
Copy Markdown

Hi @jvleeuwen00 I do not know if you are aware, but at the moment all (or nearly all) 'bergend bakjes' hold the same polygon as the 'doorstromend bakje'. This is partly due to the lack of time to make this separation, but also due to the lack of a proper polygon base layer which could be used the time down the area of the doorgaand water (+ off course the question where does the doorgaand water stop and the bergend bakje start)

@jvleeuwen00

Copy link
Copy Markdown
Collaborator Author

Hi @jvleeuwen00 I do not know if you are aware, but at the moment all (or nearly all) 'bergend bakjes' hold the same polygon as the 'doorstromend bakje'. This is partly due to the lack of time to make this separation, but also due to the lack of a proper polygon base layer which could be used the time down the area of the doorgaand water (+ off course the question where does the doorgaand water stop and the bergend bakje start)

Hi @gijsber thanks for your input. We are currently taking the LHM as it is. The division of emissions over the bergend and doorgaand nodes, as it is currently implemented, takes into account that the polygons of these nodes occupy the same space (hence the partitioning should always add up to one). So far I assumed that this will stay the same for practical reasons (e.g. determining rainfall and infiltration fluxes over the area), and that the delay between each bergend and doorgaand node would be fitted. Am I wrong to assume this and is the schematisation subject to changes in this respect?

@gijsber

gijsber commented Dec 8, 2025

Copy link
Copy Markdown

Hi @jvleeuwen00, I think such schematisation update is relative low on our priority list for the moment.

jvleeuwen00 and others added 3 commits December 10, 2025 11:30
…t ANIMO2Delwaq.py. Update line 47 (description) in combined_testing.py to indicate an additional line of input needs to be provided to the Delwaq input file to include ANIMO loads.
@gpijcke

gpijcke commented Dec 17, 2025

Copy link
Copy Markdown

@sibrenloos : I have just added ANIMO2Delwaq.py to this branch. Can you review this script?

@evetion

evetion commented Mar 21, 2026

Copy link
Copy Markdown
Member

We should update this PR once Deltares/Ribasim#2990 is merged, so we don't need to generate the loads here.

@evetion

evetion commented Mar 25, 2026

Copy link
Copy Markdown
Member

@jvleeuwen00 I made some small changes to notebooks/ribasim_delwaq/ER_to_delwaq/ER_data_conversion_delwaq.py:

  • Uploaded the required files to the Good Cloud
  • Used those files
  • Wrote an intermediate output file for caching/review
  • Added commented out example of how writing the Loads table would work (once Add loads table. Ribasim#2990 is merged).

I'll do the notebooks/ribasim_delwaq/ANIMO_to_delwaq/ANIMO2Delwaq.py next.

jvleeuwen00 and others added 9 commits April 8, 2026 10:56
…moved some comments and added some additional comments for clarity.
…erlap fraction function, as the field 'meta_categorie' (bergend, doorgaand) is currently not included in the hws model, which caused an error.
 ER conversion: removed manual include file writing from, which now adds to ribasim data structure.
 combined_testing: Removed writing concentration output to arrow in combined_testing, as this is already covered by parse(to_input) --> netcdf, removed manual node coupling list for block 6, this is now handled by generate.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants