nens / threedi-schema

The schema of 3Di schematization files
MIT License
0 stars 0 forks source link

Consistency in relation between use_* fields and corresponding tables #129

Open leendertvanwolfswinkel opened 1 month ago

leendertvanwolfswinkel commented 1 month ago

This applies to the following settings:

Model settings

use_groundwater_flow -> groundwater use_groundwater_storage -> groundwater use_interception -> interception use_interflow -> interflow use_simple_infiltration -> simple_infiltration use_vegetation_drag_2d -> vegetation_drag_2d

Simulation template settings

use_0d_inflow -> surface and dry_weather_flow use_structure_control -> table_control and memory_control

margrietpalm commented 1 week ago

Please clarify:

If the source attributes for the tables are all empty (NULL or ''), no table should be created. E.g. if v2_global_settings.interception_global and .interception_file are both empty, interception should not have any rows

Do you want an empty table of no table?

margrietpalm commented 1 week ago

Moved

- [ ] The model checker should give a INFO message if the table has data, but the corresponding `use_*` attribute is False
- [ ]  The model checker should give an ERROR if the `use_*` attribute is True, but the corresponding table has no data

to https://app.zenhub.com/workspaces/team-3di-5ef60eff1973dd0024268b90/issues/gh/nens/threedi-modelchecker/412

leendertvanwolfswinkel commented 1 week ago

Do you want an empty table of no table?

Empty table

margrietpalm commented 2 days ago

It looks like the migration needs to be refined. Currently, this happens:

property migration rule for use_ migration rule for table
use_interflow True if interflow_settings_id is not NULL or "" remove rows not matching interflow_settings_id
use_structure_control True if control_group_id is not NULL or "" remove rows not matching control_group_id
use_simple_infiltration True if simple_infiltration_settings_id is not NULL or "" remove rows not matching simple_infiltration_settings_id
use_vegetation_drag_2d True if vegetation_drag_settings_id is not NULL or "" remove rows not matching vegetation_drag_settings_id
use_groundwater_storage True if groundwater_settings_id is not NULL or "" remove rows not matching groundwater_settings_id
use_groundwater_flow True if groundwater.groundwater_hydraulic_conductivity and groundwater.groundwater_hydraulic_conductivity_file are not NULL nothing
use_interception True if interception.interception != NULL and interception.interception_file is not NULL or "" nothing
use_0d_inflow copied from global surface and dry weather flow are not populated when use_0d_inflow is False

This should be extended with:

margrietpalm commented 2 days ago

@leendertvanwolfswinkel can you confirm that the setting of use_groundwater_flow is done correctly. Currently this sql is executed:

        UPDATE model_settings
        SET use_groundwater_flow = CASE
            WHEN (SELECT groundwater_hydraulic_conductivity FROM groundwater LIMIT 1) IS NOT NULL OR (SELECT groundwater_hydraulic_conductivity_file FROM groundwater LIMIT 1) IS NOT NULL THEN 1
            ELSE 0
        END;

And this happens after setting use_groundwater_storage and removing any rows in groundwater that do not match the id. This implies that if use_groundwater_storage = False, use_groundwater_flow = False as well.

margrietpalm commented 2 days ago

@leendertvanwolfswinkel are you really sure about this?

If they are not empty, the corresponding use_* attribute should be set to True

Imagine a user sets use_* to False but doesn't remove the relating data, this would change that setting back to True. And I don't think this is the intended behavior.