Apply sign on gradients, not on water levels #1070
Conversation
| @@ -0,0 +1,41 @@ | |||
| 2024-11-29:08:57 INFO [scheduler.py:1611] Waiting for redis to become available | |||
There was a problem hiding this comment.
Do we need all those log files in the repo?
There was a problem hiding this comment.
No, I can clean it up a bit
| @@ -0,0 +1,114 @@ | |||
| { | |||
There was a problem hiding this comment.
Do we need this file for the tests?
There was a problem hiding this comment.
Having the flow summary is nice, because it contains the information about the simulation (which model was used, sim id, etc.)
| @@ -764,7 +752,7 @@ def gradients( | |||
| gr=gr, | |||
| flowline_ids=flowline_ids, | |||
| node_variable="s1", | |||
| aggregation_sign=aggregation_sign, | |||
| aggregation_sign=AggregationSign(short_name="net", long_name="Net"), # apply sign to gradient, not to s1 | |||
There was a problem hiding this comment.
Is a "net" aggregation basically doing nothing?
There was a problem hiding this comment.
well, the AggregationSign does nothing, so in that sense you are right. But the AggregationMethod still does its job. e.g. it is perfectly valid to calculate the net sum, but also valid to calculate the positive sum or negative sum. Or absolute max etc.
| @@ -779,7 +767,11 @@ def gradients( | |||
| levels_end = levels.T[end_node_indices] | |||
| distances = get_lengths(lines) | |||
| gradients = (levels_end - levels_start).T / distances | |||
| return gradients, time_intervals | |||
|
|
|||
| # apply sign | |||
There was a problem hiding this comment.
Does this also apply for "bed_level"?
There was a problem hiding this comment.
The AggregationVariable that has bed_level as short_name as defined in constants does not support having a sign, so this not super relevant. But I will add some code to only apply the sign if the aggregation variable is water level.
| from .constants import AGGREGATION_VARIABLES, AGGREGATION_METHODS | ||
|
|
||
|
|
||
| def test_hybrid_time_aggregate_gradient(): |
There was a problem hiding this comment.
are you sure this test is collected (test this by adding an "assert False")?
There was a problem hiding this comment.
def test_hybrid_time_aggregate_gradient():
> assert False
E assert False
FAILED utils/threedi_result_aggregation/test_threedi_result_aggregation.py::test_hybrid_time_aggregate_gradient
There was a problem hiding this comment.
So yes it is collected :)
leendertvanwolfswinkel
deleted the
leendert-result-aggregation-gradient-bug
branch
No description provided.