Move validation out of execute methods #1375

emlys · 2023-08-16T23:13:48Z

Description

Removed calls to validate from execute
Removed validation from execute that's redundant to what validate does
For Wind Energy, Recreation, and NDR, moved custom validation from execute into validate. I briefly tested the UI for these models to confirm that the new validation messages show up.

I did not do anything about custom validation that happens in functions other than execute - trying to keep the scope small.

Checklist

Updated HISTORY.rst and link to any relevant issue (if these changes are user-facing)
Updated the user's guide (if needed)
Tested the affected models' UIs (if relevant)

…atcap#1373

davemfish

@emlys thanks for helping us standardize our approaches across models. I made a number of comments here that pretty much all boil down to this one opinion:

execute should behave as expected (or raise an exception) whether or not the args have been validated. Because Python API users aren't required to call validate before calling execute.

I might be okay with penalizing them with more cryptic errors or longer waits before an exception, but we should avoid cases where invalid args could yield unexpected results.

And if we don't think Python users should be penalized that way, then we could consider adopting the approach of always calling validate at the start of execute. Of course the cost there is a duplicated validate call for Workbench users or anyone else who already validated.

Let me know what you think. I know the rest of the team already discussed this.

davemfish · 2023-08-21T12:23:42Z

src/natcap/invest/hra.py

@@ -509,13 +509,9 @@ def execute(args):

    if args['risk_eq'].lower() == 'multiplicative':
        max_pairwise_risk = max_rating * max_rating
-    elif args['risk_eq'].lower() == 'euclidean':
+    else:  # euclidean


In my opinion this code now only makes sense when we know for certain that validate was called first. And we don't know that for python API users. I don't see any downside to keeping in the error-handling here. It would catch a typo like 'mltiplicative'.

I think this is different from other cases where not doing validation in execute just means the user has to wait longer before hitting an error. So it's like, "don't call validate at your own risk". But in this case something different & unexpected could happen.

src/natcap/invest/recreation/recmodel_client.py

davemfish · 2023-08-21T12:52:34Z

src/natcap/invest/scenic_quality/scenic_quality.py

@@ -795,7 +791,7 @@ def _valuation(distance, visibility):
                    weight * visibility[valid_pixels]))
            return valuation

-    elif valuation_method == 'exponential':
+    else:  # exponential


In my opinion it's better to keep this as elif valuation_method == 'exponential' and let the subsequent NameError arise when trying to call an undefined _valuation(). Otherwise exponential would be applied to a typo like logarthmic. Or the other option is to keep in the validation step that happens much earlier.

davemfish · 2023-08-21T12:55:03Z

src/natcap/invest/urban_nature_access.py

@@ -922,19 +922,13 @@ def execute(args):
        # Build an iterable of plain tuples: (lucode, search_radius_m)
        lucode_to_search_radii = list(
            urban_nature_attrs[['search_radius_m']].itertuples(name=None))
-    elif args['search_radius_mode'] == RADIUS_OPT_POP_GROUP:
+    else:  # population group mode


Same issue as in a couple other place: I don't think we can assume that validate was called, and we don't want to run the wrong block of code instead of raising an exception.

davemfish · 2023-08-21T12:55:34Z

src/natcap/invest/urban_nature_access.py

        pop_group_table = utils.read_csv_to_dataframe(
            args['population_group_radii_table'],
            MODEL_SPEC['args']['population_group_radii_table'])
        search_radii = set(pop_group_table['search_radius_m'].unique())
        # Build a dict of {pop_group: search_radius_m}
        search_radii_by_pop_group = pop_group_table['search_radius_m'].to_dict()
-    else:


This goes with line 925

tests/test_recreation.py

davemfish · 2023-08-21T13:15:40Z

tests/test_routedem.py

-        }
-
-        RouteDEMTests._make_dem(args['dem_path'])
-        with self.assertRaises(RuntimeError) as cm:


It looks like this test would still make sense if it checked for KeyError instead of RuntimeError. I think it's important that a bad algorithm raises some kind of error, so might as well keep this test around in my opinion.

davemfish · 2023-08-21T13:18:05Z

tests/test_scenic_quality.py

-            'n_workers': -1,
-        }
-
-        with self.assertRaises(ValueError):


Similar comment as in test_routedem. This would now raise a NameError, I think. And it would be bad if it raised no error at all, so it might be nice to just keep the test.

davemfish · 2023-08-21T13:19:27Z

tests/test_urban_nature_access.py

@@ -807,22 +807,6 @@ def test_polygon_overlap(self):
            [polygon_1, polygon_2, polygon_3], vector_path, wkt, 'GeoJSON')
        self.assertTrue(urban_nature_access._geometries_overlap(vector_path))

-    def test_invalid_search_radius_mode(self):


Related to my comment in the model, we may want to keep this test.

davemfish · 2023-08-21T13:46:32Z

src/natcap/invest/recreation/recmodel_client.py

-            "One or more of the projections in the table did not match the "
-            "projection of the base vector")
+        return ("One or more of the projections in the table did not match "
+                "the projection of the base vector")


Could we include base_vector_path in the message instead of saying "base vector"? I'm worried it's not obvious that "base vector" is the AOI vector. And maybe the same for using table_path instead of "table"?

davemfish

Thank you @emlys ; lots of useful improvements here!

emlys added 8 commits August 15, 2023 11:53

remove validate calls from execute functions natcap#1373

c9054d6

remove other duplicated validation from execute methods natcap#1373

0073cf0

moving rec model validation into validate method natcap#1373

8a90e7f

return validation messages rather than raising error natcap#1373

ea1a9dd

Merge branch 'main' into task/1373

a805df8

fix a few bugs introduced in merge and update tests for natcap#1373

0e0c7b1

fix ndr test natcap#1373

f82729b

history note for natcap#1373

d98528e

emlys self-assigned this Aug 17, 2023

emlys marked this pull request as ready for review August 17, 2023 23:39

emlys requested a review from davemfish August 17, 2023 23:39

emlys added 2 commits August 17, 2023 17:14

check if inputs are sufficent and valid before doing extra validation n…

e5a2292

…atcap#1373

wind energy validation: read csv directly with pandas to avoid errors n…

7a2ad45

…atcap#1373

davemfish requested changes Aug 21, 2023

View reviewed changes

emlys added 2 commits September 6, 2023 15:50

pr feedback: leave in some redundant error handling in execute

be89b2a

Merge branch 'main' into task/1373

f693dc4

emlys requested a review from davemfish September 6, 2023 23:30

davemfish approved these changes Sep 7, 2023

View reviewed changes

davemfish merged commit 3f44e18 into natcap:main Sep 7, 2023
21 checks passed

emlys deleted the task/1373 branch October 3, 2024 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move validation out of execute methods #1375

Move validation out of execute methods #1375

emlys commented Aug 16, 2023 •

edited

Loading

davemfish left a comment

davemfish Aug 21, 2023

davemfish Aug 21, 2023

davemfish Aug 21, 2023

davemfish Aug 21, 2023

davemfish Aug 21, 2023

davemfish Aug 21, 2023

davemfish Aug 21, 2023

davemfish Aug 21, 2023

davemfish left a comment

Move validation out of execute methods #1375

Move validation out of execute methods #1375

Conversation

emlys commented Aug 16, 2023 • edited Loading

Description

Checklist

davemfish 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davemfish left a comment

Choose a reason for hiding this comment

emlys commented Aug 16, 2023 •

edited

Loading