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

include overflows in symbology #166

Merged
merged 11 commits into from
May 7, 2024

Conversation

cymed
Copy link
Contributor

@cymed cymed commented Feb 15, 2024

There are several situations (i.E. a stormwater composite tank with a preceding separating structure) where a wastewater structure only has overflows as topological connections. In these situations, we should also set a _usage_current dependent on the incoming overflow. This PR includes them in the updating process for wastewater nodes.

See also https://github.com/QGEP/datamodel/pull/225/files

@cymed cymed requested a review from sjib February 21, 2024 16:09
@cymed cymed requested review from ponceta and removed request for sjib March 5, 2024 15:03
@sjib sjib added the enhancement New feature or request label Apr 17, 2024
@cymed cymed added this to the TEKSI Wastewater 2024.0 milestone Apr 17, 2024
@cymed cymed requested review from ponceta and removed request for ponceta April 17, 2024 11:40
@cymed cymed closed this Apr 17, 2024
@cymed cymed reopened this Apr 17, 2024
@cymed
Copy link
Contributor Author

cymed commented Apr 17, 2024

@ponceta ready for review

@ponceta
Copy link
Member

ponceta commented Apr 23, 2024

It looks good, does it works good too?

Have you tried it on with a data set? My main concern is the performance, if it's green let's go!

@cymed
Copy link
Contributor Author

cymed commented Apr 25, 2024

It looks good, does it works good too?

Have you tried it on with a data set? My main concern is the performance, if it's green let's go!

It works with real data. For a town of 6000 inhabitants, I feel no apparent drop in QGIS.
For performance, I have tested the SELECT DISTINCT ON ... bit with _all=true on a small db of 2'570 wastewater nodes.

Before:
Planning Time: 3.110 ms
Execution Time: 77.668 ms

New:
Planning Time: 12.957 ms
Execution Time: 129.622 ms

For a single wastewater node (i.e. when altering a wastewater structure)
Before:
Planning Time: 3.188 ms
Execution Time: 0.216 ms

New:
Planning Time: 8.382 ms
Execution Time: 0.525 ms

If the performance drop bothers you I suggest to store _usage_current, _status and _function_hierarchic on tww.overflow and grab it from there. Potential problem of circular triggers when we do that though

@ponceta
Copy link
Member

ponceta commented Apr 29, 2024

I'm OK to go with that but I would enjoy a good schema in the doc on how it works and what special structures can be well modelized with that, otherwise it is quite an additionnal complexity in terms of symbology.

I think it is mainly on the overflows "new" mechanics and it would be good if one can achieve it without being a wastewater datamanager certified engineer. (otherwise I'm quite excited to see how it looks in real life examples.)

cymed and others added 4 commits April 30, 2024 10:29
We improve performance by separating overflow-based and channel-based functionalities (fewer unnecessary left joins)
@cymed
Copy link
Contributor Author

cymed commented Apr 30, 2024

The last pushes improve performance and readability. The only change to the original function is that I included the wn's corresponding ws for status.

All the overflow specific stuff is moved into a separated function, making it much faster as it left joins the nodes onto an overflow instead of left joining the overflow on the node.

I'm OK to go with that but I would enjoy a good schema in the doc on how it works and what special structures can be well modelized with that, otherwise it is quite an additionnal complexity in terms of symbology.

I think it is mainly on the overflows "new" mechanics and it would be good if one can achieve it without being a wastewater datamanager certified engineer. (otherwise I'm quite excited to see how it looks in real life examples.)

I don't think we need another docs. It only affects symbology on wastewater nodes for those who follow the vsa wiki. The rest won't notice.

Here is a real-life example, where there is an separating structure is connected to a combined sewer overflow with retention volume , which in turn has an overflow into the water body. Without the Pull request, the CSO has no usage_current and remains black
grafik

@ponceta can I merge?

@sjib
Copy link
Contributor

sjib commented Apr 30, 2024

@cymed @ponceta May be we could include this screenshot in this chapter: https://qgep.github.io/docs/user-guide/How-To/index.html#hydraulic-modeling-of-an-overflow-prank-weir-leapingweir-pump

@ponceta ponceta closed this May 7, 2024
@ponceta ponceta reopened this May 7, 2024
@ponceta ponceta merged commit f0a5391 into teksi:main May 7, 2024
13 checks passed
@cymed cymed deleted the include_overflows_in_symbology branch May 8, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants