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

Water Tower Component #107

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

SimonRubenDrauz
Copy link
Collaborator

Introducing new component: Water Tower

lstarost and others added 4 commits July 28, 2020 15:22
- renamed reservoir in water tower
- adaption of create and component
- adaption of test
@SimonRubenDrauz SimonRubenDrauz requested a review from jkisse August 3, 2020 08:45
@SimonRubenDrauz SimonRubenDrauz changed the title Slowlow/develop Water Tower Component Aug 3, 2020
@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #107 into develop will increase coverage by 0.42%.
The diff coverage is 90.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #107      +/-   ##
===========================================
+ Coverage    90.67%   91.09%   +0.42%     
===========================================
  Files           59       60       +1     
  Lines         3248     3224      -24     
===========================================
- Hits          2945     2937       -8     
+ Misses         303      287      -16     
Impacted Files Coverage Δ
pandapipes/create.py 97.60% <75.00%> (ø)
pandapipes/create_toolbox.py 100.00% <100.00%> (ø)
pandapipes/std_types/std_type_toolbox.py 91.30% <0.00%> (-3.94%) ⬇️
pandapipes/std_types/std_type.py 81.94% <0.00%> (-2.12%) ⬇️
pandapipes/io/file_io.py 81.25% <0.00%> (+1.70%) ⬆️
pandapipes/io/io_utils.py 78.08% <0.00%> (+9.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e208959...e044b08. Read the comment docs.

Comment on lines 52 to 53
press = density * (water_towers.height_m.values - node_pit[index_wt, HEIGHT]) * \
GRAVITATION_CONSTANT / P_CONVERSION * water_towers.in_service.values
Copy link
Collaborator

@jkisse jkisse Aug 4, 2020

Choose a reason for hiding this comment

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

@SimonRubenDrauz What exactly is the definition of height_m? My guess was the height of the water column in the tower. However, if the junction's height is subtracted, this could lead to negative height, couldn't it?
I'd find it confusing if height_m was in "meters above sea level" but I do not know the conventions.
As a suggestion, maybe it should be renamed to tower_height_m. A scaling factor could be introduced to derive water_column_height_m which then serves as input for the static pressure (rhogwater_column_height_m)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It actually should be the height above sea level. But I agree. It really seems confusing. It is a convention in SINCAL. But I would change it in our case taking only the height of the water_column.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the adaption! However, thinking about it twice, maybe this leads to a problem regarding communicating vessels. (see my latest comment.) This could be the reason why Sincal uses absolute height.


class WaterTower(NodeElementComponent):
"""

Copy link
Collaborator

@jkisse jkisse Aug 4, 2020

Choose a reason for hiding this comment

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

Could you add some information on the pictured model and important limitations and assumptions?
It seems to me like an infinite diameter is assumed (no change in water column height on withdraw or feed-in), is that correct?
How does it differ from an ext_grid, apart from the different formulations of pressure, and to which extend does it overlap?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I will do so.
There are not many differences compared to a ext_grid. The pressure is determined by the height of the water column. It is assumed that the water tower capacity is unlimited and the height of the water column remains constant, correct!
I agree that we could think of making a new parent component for ext_grid and water_tower. But for now I would leave it like this!

sdrauz added 3 commits August 4, 2020 17:44
Comment on lines 52 to 55
press = density * water_towers.height_m.values * \
GRAVITATION_CONSTANT / P_CONVERSION * water_towers.in_service.values
juncts_p, press_sum, number = _sum_by_group(junction.values, press,
np.ones_like(press, dtype=np.int32))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure whether this is physically possible. The effect of communicating vessels (dt: "kommunizierende Röhren") is not considered, is it?
I'd suggest limiting the number of water towers to 1 per net or raise a warning when towers with different water height are connected.

@dlohmeier
Copy link
Collaborator

To me it is a bit questionable whether this component makes sense in this form, especially due to the similarity to the ext_grid that @jkisse mentioned. There are several issues connected to this component:

  1. Why is it so important to create a whole component just for the sake of one equation that can easily be implemented by a user himself? If this is implemented as a controller with an ext_grid, it is possible to adapt the pressure of the ext_grid with the help of the water height, and you could even implement a control loop that adapts the average height during a time series simulation based on the water extraction.
  2. If you implement this as a component, what do you do if an ext_grid is connected to the same junction as a water tower and they don't show the same pressure? What you implemented by now only divides the mass flows over those components in the results, but the pressure PINIT can only be set in each component individually, so the latest initialized component determines the pressure.
    Maybe this is the time where we should think about how to aggregate different components and their behavior in one function, which would also lead to a speed-up if e.g. all pipes and valves can be calculated as one part of the pit. Yet, this adds more complexity to our whole structure which is already extremely complex.

@SimonRubenDrauz
Copy link
Collaborator Author

SimonRubenDrauz commented Aug 10, 2020

Thanks for your feedback. I adapted now everything. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants