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:

Ribasim/core/src/schema.jl

Lines 278 to 294 in 98df891

	@version UserDemandStaticV1 begin
	node_id::Int32
	active::Union{Missing, Bool}
	demand::Union{Missing, Float64}
	return_factor::Float64
	min_level::Float64
	priority::Int32
	end
	@version UserDemandTimeV1 begin
	node_id::Int32
	time::DateTime
	demand::Float64
	return_factor::Float64
	min_level::Float64
	priority::Int32
	end

The default value of 0 I think comes from here:

Ribasim/python/ribasim/ribasim/schemas.py

Lines 246 to 261 in 98df891

	class UserDemandStaticSchema(_BaseSchema):
	node_id: Series[Int32] = pa.Field(nullable=False, default=0)
	active: Series[pa.BOOL] = pa.Field(nullable=True)
	demand: Series[float] = pa.Field(nullable=True)
	return_factor: Series[float] = pa.Field(nullable=False)
	min_level: Series[float] = pa.Field(nullable=False)
	priority: Series[Int32] = pa.Field(nullable=False, default=0)
	class UserDemandTimeSchema(_BaseSchema):
	node_id: Series[Int32] = pa.Field(nullable=False, default=0)
	time: Series[Timestamp] = pa.Field(nullable=False)
	demand: Series[float] = pa.Field(nullable=False)
	return_factor: Series[float] = pa.Field(nullable=False)
	min_level: Series[float] = pa.Field(nullable=False)
	priority: Series[Int32] = pa.Field(nullable=False, default=0)

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.