Skip to content
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

[Requires Testing] Add label to reach point #219

Closed
wants to merge 99 commits into from

Conversation

cymed
Copy link
Contributor

@cymed cymed commented May 15, 2023

In order to solve #9, a first step is to add a _label to reach point. The update procedure is entered in the update process of the wastewater structure labels. I tested the section for reach_points and for wastewater_structures separately, and they work. However, putting the code into one function as in the push probably causes recursions. Comments and ideas appreciated.

@cymed cymed changed the title Add label to reach point [WIP] Add label to reach point May 15, 2023
@cymed
Copy link
Contributor Author

cymed commented May 15, 2023

Same idea as in #212, but I changed the branch name, which closed the other PR.

@sjib sjib mentioned this pull request May 19, 2023
2 tasks
solves stack overflow, trigger functionality pending
@cymed
Copy link
Contributor Author

cymed commented Jun 20, 2023

Current status: trigger functions are working, but triggers themselves are not set yet

@ponceta ponceta closed this Jul 17, 2023
@ponceta ponceta reopened this Jul 17, 2023
@ponceta
Copy link
Member

ponceta commented Aug 25, 2023

CI says : https://github.com/QGEP/datamodel/actions/runs/5948505152/job/16132413018?pr=219#step:15:7166

Applying delta 1.6.1 update_rp_label... dict is not a sequence

@cymed
Copy link
Contributor Author

cymed commented Oct 2, 2023

What is striking me in this WIP is the time taken by the CI Check. There might be an infinite loop

@ponceta
Copy link
Member

ponceta commented Oct 2, 2023

What is striking me in this WIP is the time taken by the CI Check. There might be an infinite loop

Have you tried it on a real database or demo-data?

From Alpha testing, there was an _all substituted
Text compares lead to performance issues, so the function now uses arrays of feasible ws_status and ch_function_hierarchic. Added benefit that they can be overriden
@cymed
Copy link
Contributor Author

cymed commented Oct 10, 2023

@ponceta @sjib all labels are now stored in qgep_od.labels and linked to their respective wastewater structure / reach point by obj_id. Altering a label should therefore no longer lead to a trigger cascade. How do you want to proceed concerning 1.6.1?

Also I didn't check if there are functions (i.e. export functions) referencing qgep_od,wastewater_structure.*_label directly (and not via qgep_od.vw_qgep_wastewater_structure)

@ponceta
Copy link
Member

ponceta commented Oct 10, 2023

This is now a clear evolution feature and requires a good testing on real data sets. To be checked also if the Interlis Import/Export would be adapted. And tested in terms of performance.

@cymed cymed modified the milestone: 2023.0 Oct 10, 2023
@sjib
Copy link
Contributor

sjib commented Oct 11, 2023 via email

@cymed
Copy link
Contributor Author

cymed commented Oct 11, 2023

I agree. I suggest that the PR will not be merged into release 1.6.1

To do:

  • Create Feature request for second part of VSA DSS 2020 conformity
  • include scalability checks for TEKSI wastewater into said FR
  • adapt this PR to be included together with said FR
  • Do extensive testing in said FR

If @sjib and @ponceta agree on thie procedure, I would close the PR for the qgep datamodel

@cymed cymed requested review from sjib and removed request for urskaufmann October 11, 2023 11:27
@cymed cymed marked this pull request as draft October 13, 2023 12:00
COMMENT ON COLUMN qgep_od.wastewater_structure._bottom_label IS 'stores the bottom altitude to be used for labelling, not part of the VSA-DSS data model
added solely for QGEP';

added solely for TEKSI wastewater';

-- this column is an extension to the VSA data model and puts the _function_hierarchic in order
ALTER TABLE qgep_vl.channel_function_hierarchic ADD COLUMN order_fct_hierarchic smallint;
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/teksi/Home/wiki/TEKSI-Developer-Guide#custom-fields
Does this also need an underscore _order_fct_hierarchic ?

Copy link
Contributor Author

@cymed cymed Oct 15, 2023

Choose a reason for hiding this comment

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

Imho no. It is not dependent of another table.
In the logic of #219 (comment) it would be x_order_fct_hierarchic

@@ -51,6 +35,15 @@ UPDATE qgep_vl.channel_function_hierarchic SET order_fct_hierarchic=12 WHERE cod
UPDATE qgep_vl.channel_function_hierarchic SET order_fct_hierarchic=11 WHERE code=5074;
UPDATE qgep_vl.channel_function_hierarchic SET order_fct_hierarchic=14 WHERE code=5075;

-- this column is an extension to the VSA data model and defines whether connected channels are included in inflow/outflow labeling based on function_hierarchic
ALTER TABLE qgep_vl.channel_function_hierarchic ADD COLUMN include_in_ws_labels boolean DEFAULT FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is a custom field usr_xxx not equal to a db extension by a single user (not included in github and pum)

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it is not totally clear if we have (here)
a) calculated fields -> _ would need an underscore
b) custom field -> usr_xxx
c) something new - an additional TEKSI needed field, that is not part of VSA-DSS but not realy "calculated" and
d) do we distinguish if this is done in a view of the model or in the view of the project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me it is c)
A Teksi-specific extension independent of other tables. My suggestion is to prefix these cases x_ (eXtension).



-- this column is an extension to the VSA data model and defines whether connected channels are included in inflow/outflow labeling based on function_hierarchic
ALTER TABLE qgep_vl.wastewater_structure_status ADD COLUMN include_in_ws_labels boolean DEFAULT FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the logic of #219 (comment) it would be x_include_in_ws_labels. Not a custom field (this one shouldnt be excluded from pum)

has to be updated by triggers';

-- label extensions
CREATE TABLE IF NOT EXISTS qgep_od.labels
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/teksi/Home/wiki/TEKSI-Developer-Guide#custom-fields
_labels instead of labels as it is not a VSA-DSS class

Copy link
Contributor Author

@cymed cymed Oct 16, 2023

Choose a reason for hiding this comment

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

_labels would be the field logic. We have no defined logic for extension tables.
Maybe we move it to qgep_sys?

@cymed cymed changed the title [WIP][Requires Testing] Add label to reach point [Requires Testing] Add label to reach point Oct 19, 2023
@cymed
Copy link
Contributor Author

cymed commented Feb 5, 2024

closing in favour of teksi/wastewater#85

@cymed cymed closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionnality or behaviour improving/extending user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants