Apply overrides in Storage and its superclasses #81

liuly12 · 2024-03-05T09:30:27Z

Apply overrides in Storage, Groundwater, QueueGroundwater, River, Riverreservoir and update the tests

see #79 (comment)

and delete parameters that are not used at all in the River node

and update river initialisation in other tests

barneydobson

Looks good - just some suggestions, nothing big

wsimod/nodes/nodes.py

barneydobson · 2024-03-05T12:16:09Z

wsimod/nodes/storage.py

@@ -394,7 +449,6 @@ class River(Storage):
    # TODO non-day timestep
    def __init__(
        self,
-        depth=2,


I'm not sure it's a great idea to remove arguments. As there will be many config files that specify depth for River nodes, and these will crash. Instead I would suggest to allow depth as an argument and have something like the following:

print('warning: the depth parameter is unused by River nodes because it is intended for capacity to be unbounded. It may be removed in a future version.')

added - see commit 07e196b

barneydobson · 2024-03-05T12:19:11Z

wsimod/nodes/storage.py

@@ -490,16 +543,6 @@ def __init__(
        )
        self.muptNpar = 0.001  # [kg/m2/day] nitrogen macrophyte uptake rate
        self.muptPpar = 0.0001  # 0.01, # [kg/m2/day] phosphorus macrophyte uptake rate


I can see that these aren't used - but I guess they were used in CatchWat? Are we likely to need them again? If so, maybe just copy this excerpt into #2

probably not as they are for the solids and we haven't decided which sedimentation-resuspension model we are going to integrate ... plus these parameters are not performing very well in previous case studies

wsimod/nodes/storage.py

Co-authored-by: barneydobson <[email protected]>

with warning for unuse

…eLondon/wsi into storage-overrides

Co-authored-by: barneydobson <[email protected]>

dalonsoa

Same comment than in #79 . The overrides look ok, but there's no reason to use pop while you can get the same thing with get. I do not see any reason for modifying the overrides dictionary.

liuly12 · 2024-10-02T09:01:39Z

The purpose to pop the overrides dict is to examine whether all intended overrides in the dict have been accomplished. If there are residual items left in the overridies, a warning will be reported through the following lines in the node.py, which provides a diagnosis message to the users to check.

    def apply_overrides(self, overrides: Dict[str, Any] = {}) -> None:
        """Apply overrides to the node.

        The Node does not have any overwriteable parameters. So if any
        overrides are passed up to the node, this means that there are unused
        parameters from the Node subclass, which is flagged.

        Args:
            overrides (dict, optional): Dictionary of overrides. Defaults to {}.
        """
        if len(overrides) > 0:
            print(f"No override behaviour defined for: {overrides.keys()}")

dalonsoa

I believe we already had this pop vs get discussion somewhere else and I was happy with the explanation, so feel free to go ahead with it.

Having said that, I would suggest to replace all print statements with a proper logger. That might be a task for a different PR, though.

wsimod/nodes/storage.py

dalonsoa · 2024-10-04T04:53:42Z

wsimod/nodes/storage.py

+            print(
+                "ERROR: specifying area is depreciated in overrides \
+		for river, please specify length and width instead"
+            )


Use exceptions.

Added runtime errors - is this what is meant?

Exactly. You could also use ValueError. There are several built-in exception you can sue. Pick the most appropriate one for each case. https://docs.python.org/3/library/exceptions.html

dalonsoa · 2024-10-04T04:53:54Z

wsimod/nodes/storage.py

+            print(
+                "ERROR: specifying capacity is depreciated in overrides \
+		for river, it is always set as unbounded capacity"
+            )


dalonsoa · 2024-10-04T04:54:47Z

wsimod/nodes/storage.py

+            print(
+                "warning: the depth parameter is unused by River nodes because it is \
+		intended for capacity to be unbounded. It may be removed in a future version."
+            )


Co-authored-by: Diego Alonso Álvarez <[email protected]>

Update decays overrides in tanks

b6b164d

see #79 (comment)

liuly12 linked an issue Mar 5, 2024 that may be closed by this pull request

Storage overrides #80

Closed

liuly12 added 6 commits March 5, 2024 09:34

Apply overrides in Storage

426ea31

Apply overrides in Groundwater

d6fa53a

Apply overrides in QueueGroundwater

eb05cd9

Apply overrides in River

c0a1921

and delete parameters that are not used at all in the River node

Apply overrides in RiverReservoir

df1e3ec

Add override tests

1540c7f

and update river initialisation in other tests

liuly12 requested a review from barneydobson March 5, 2024 09:58

barneydobson approved these changes Mar 5, 2024

View reviewed changes

liuly12 and others added 4 commits March 5, 2024 14:16

Warn that changing area won't change capacity in Tank

b9e7ec0

Co-authored-by: barneydobson <[email protected]>

Restore the depth parameter

07e196b

with warning for unuse

Merge branch 'storage-overrides' of https://github.com/ImperialColleg…

f5eaf1c

…eLondon/wsi into storage-overrides

Tidy

8cd6b4e

Co-authored-by: barneydobson <[email protected]>

liuly12 mentioned this pull request Mar 5, 2024

Refactor river biochemical processes #2

Open

bug fix

da7c538

liuly12 requested a review from dalonsoa April 10, 2024 09:12

dalonsoa requested changes Apr 18, 2024

View reviewed changes

liuly12 and others added 3 commits July 26, 2024 12:17

Merge branch 'main' into storage-overrides

0a63072

pre-commit reformatted

8ede84a

merge

bf716c4

barneydobson marked this pull request as ready for review October 1, 2024 13:54

fix tests

d59a6e6

Merge branch 'main' into storage-overrides

9be6ab8

dalonsoa approved these changes Oct 4, 2024

View reviewed changes

liuly12 and others added 4 commits October 9, 2024 11:02

Use set rather than list

ce74892

Co-authored-by: Diego Alonso Álvarez <[email protected]>

Raise runtime error and add corresponding test

ca7668f

qa

bdd0b1b

qa

a5335da

liuly12 merged commit 08b0328 into main Oct 10, 2024
21 checks passed

liuly12 deleted the storage-overrides branch October 10, 2024 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply overrides in Storage and its superclasses #81

Apply overrides in Storage and its superclasses #81

liuly12 commented Mar 5, 2024 •

edited

Loading

barneydobson left a comment

barneydobson Mar 5, 2024

liuly12 Mar 5, 2024

barneydobson Mar 5, 2024

liuly12 Mar 5, 2024

dalonsoa left a comment

liuly12 commented Oct 2, 2024 •

edited

Loading

dalonsoa left a comment

dalonsoa Oct 4, 2024

liuly12 Oct 9, 2024

dalonsoa Oct 9, 2024

dalonsoa Oct 4, 2024

dalonsoa Oct 4, 2024

Apply overrides in Storage and its superclasses #81

Apply overrides in Storage and its superclasses #81

Conversation

liuly12 commented Mar 5, 2024 • edited Loading

barneydobson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalonsoa left a comment

Choose a reason for hiding this comment

liuly12 commented Oct 2, 2024 • edited Loading

dalonsoa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuly12 commented Mar 5, 2024 •

edited

Loading

liuly12 commented Oct 2, 2024 •

edited

Loading