Priority parameter should be non-optional

[Jingru923]

What Recently in solving issue #1724 , it has come to me that even thought the priority of user demand is not given, the whole program can still be run.

As this is not a desire behavior, investigation into the reason is needed.

[gijsber]

Priority paramneter is only required when use_allocation=true. If you do not use allocation, the priority parameter can be optional.

[Jingru923]

Thanks @gijsber . In this case the documentation should be updated since it gave not only me but also other user and developer some confusion.

However, if I run models with allocation active and user demand node without specifying priority, the model can still be run. This is the same situation even for the models with main network. This should still be looked into.

[gijsber]

if you have a model with allocation active and user demand node without specifying priority, you should get a validation error

[Jingru923]

I agree with you @gijsber . An error should be given in this case.

What worried me is that the model can still run even though the mode has such error. This mean that in the current implementation, priority parameter is not needed. We should change the implementation into one that actually make use of the priority, then give an error if priority is missing.

[Jingru923]

A finding on this issue. When reading the demand nodes into geodata based, a column "priority" is created and all the values inside this column are initialized as 0. Therefore, even if no priority parameter are given to the demand nodes, the model can still run. The nodes would have priorities of 0.

To avoid that, I wrote a validation function for all the demand nodes. If allocation is active, priorities is a mandatory field. If priority is given but allocation is not active, a warning message will be display but the program will still run

[SouthEndMusic]

We don't have warning messages thus far, because @visr is not a fan of them. I think the priorities not being used when allocation is not active does belong in the documentation but does not need the warning message.

Note that the priority is mandatory in the core: https://github.com/Deltares/Ribasim/blob/98df891d5f75d28074dda089f1cc086cc3d150db/core/src/schema.jl#L278-L294

The default value of 0 I think comes from here: https://github.com/Deltares/Ribasim/blob/98df891d5f75d28074dda089f1cc086cc3d150db/python/ribasim/ribasim/schemas.py#L246-L261

Not sure how that ends up in that automatically generated file, maybe it's standard for Int32 parameters.

Currently priorities < 1 are accepted in the core. That's quite a handy design, because then you don't have to shift all other priorities if you want to add a new most important priority level. A downside of this is that the default priority = 0 is accepted as a valid priority.

I think the best way to solve this is as follows:

• make priority for UserDemand optional and get rid of the default value of 0;
• throw an error in the core if allocation is active and priority is not specified.

[Jingru923]

It is easier to do validation with the default value. I suggest that we keep it for now.