Bpoly feature #69

matthiasschaub · 2021-06-28T14:25:07Z

Description

Change type of attribute bpolys to be GeoJSON.Feature.
Input bpolys to API can be of type GeoJSON.Feature with the geometry type Polygon or MultiPolygon. Input can also only contain the geometry.
Input file to API can be any valid GeoJSON. Geometry type has to be Polygon or MultiPolygon
Improve type hints.

Corresponding issue

Closes #9

Checklist

support FeatureCollection with only one Feature (again)
I have updated my branch to main (e.g. through git rebase main)
My code follows the style guide and was checked with pre-commit before committing
I have commented my code
I have added sufficient unit and integration tests
I have updated the CHANGELOG.md

CHANGELOG.md

workers/ohsome_quality_analyst/api.py

CHANGELOG.md

joker234 · 2021-07-05T14:11:57Z

workers/ohsome_quality_analyst/api.py

+def int_or_str_param_type(value: str) -> Union[int, str]:
+    """Parse parameter as Interger otherwise parse as String"""
+    try:
+        return int(value)
+    except (ValueError, TypeError):
+        if isinstance(value, str):
+            return value
+        else:
+            raise ValueError("Given parameter is not a valid integer or string")


Do you have an example where this could be required? As far as I see, this is only used for the feature_id. The feature id given by a GET/POST request should always be given as a string, afaik. If you want to prevent the parameter from being optional, I would check/set this with fastAPI.

Yes you are right. The else statement is never reached. Except maybe when None is passed as argument. But before calling this function it is checked whether feature id is none or not.

I meant in general, why do you want a variable which is either int or str? In the database you can only handle one of them, right? So you can handle all identifier in OQT as string, or did I miss a problem with that? I think it would be much easier to focus on one type for identifier 😉

Ah, now I see what you mean.
Since #37 feature_id can be str or int depending on what feature_id_field is.

E.g.

# int in the database oqt create-indicator --fid-field ogc_fid --feature-id 1 # str in the database oqt create-indicator --fid-field name --feature-id heidelberg

What do you think about this behavior?

workers/ohsome_quality_analyst/api.py

workers/ohsome_quality_analyst/base/indicator.py

workers/ohsome_quality_analyst/geodatabase/client.py

workers/ohsome_quality_analyst/api.py

joker234 · 2021-07-05T16:35:28Z

workers/ohsome_quality_analyst/api.py

+def int_or_str_param_type(value: str) -> Union[int, str]:
+    """Parse parameter as Interger otherwise parse as String"""
+    try:
+        return int(value)
+    except (ValueError, TypeError):
+        if isinstance(value, str):
+            return value
+        else:
+            raise ValueError("Given parameter is not a valid integer or string")


I meant in general, why do you want a variable which is either int or str? In the database you can only handle one of them, right? So you can handle all identifier in OQT as string, or did I miss a problem with that? I think it would be much easier to focus on one type for identifier 😉

joker234

Thanks for the PR and the additional tests 👍 I made some additional comments, but in general, the PR looks good.

workers/ohsome_quality_analyst/cli.py

workers/ohsome_quality_analyst/geodatabase/client.py

workers/ohsome_quality_analyst/indicators/ghs_pop_comparison_buildings/indicator.py

workers/ohsome_quality_analyst/indicators/ghs_pop_comparison_roads/indicator.py

workers/tests/integrationtests/test_geodatabase.py

matthiasschaub · 2021-07-06T07:40:03Z

@joker234 Thanks for your attentive feedback! I addressed most of your feedback directly in the code. Some discussions are still open.

Parse and extract Geometry from input GeoJSON Input to the API is a valid GeoJSON. The function load_bpolys() will extract the geometry of this GeoJSON and validate it. Rename fixture file Add missing type hint Make bpolys attribute madatory for indicator class Add changes to CHANGELOG Update fixtures

after load_bpolys() has been called.

matthiasschaub marked this pull request as draft June 28, 2021 14:25

matthiasschaub force-pushed the bpoly-feature branch from 3adef2e to 21c6150 Compare June 29, 2021 06:48

matthiasschaub marked this pull request as ready for review June 29, 2021 06:48

matthiasschaub force-pushed the bpoly-feature branch from 21c6150 to 99bd795 Compare June 29, 2021 06:49

matthiasschaub mentioned this pull request Jun 29, 2021

Bpoly geometry #63

Closed

7 tasks

matthiasschaub added the waiting for review This pull request urgently needs a code review label Jun 29, 2021

matthiasschaub force-pushed the bpoly-feature branch from 99bd795 to 787f4c5 Compare June 29, 2021 06:57

matthiasschaub mentioned this pull request Jun 29, 2021

Implement __geo_interface__ for indicator class #66

Closed

6 tasks

matthiasschaub requested a review from joker234 July 5, 2021 07:58

matthiasschaub force-pushed the bpoly-feature branch from 787f4c5 to 850519a Compare July 5, 2021 13:54

joker234 reviewed Jul 5, 2021

View reviewed changes

matthiasschaub force-pushed the bpoly-feature branch from c6aa948 to 66317a1 Compare July 5, 2021 16:03

matthiasschaub requested a review from joker234 July 5, 2021 16:05

joker234 reviewed Jul 5, 2021

View reviewed changes

joker234 removed the waiting for review This pull request urgently needs a code review label Jul 6, 2021

matthiasschaub force-pushed the bpoly-feature branch from 66317a1 to d78d228 Compare July 6, 2021 07:40

matthiasschaub requested a review from joker234 July 6, 2021 07:40

matthiasschaub force-pushed the bpoly-feature branch 2 times, most recently from 7986c85 to 94d0737 Compare July 6, 2021 08:59

matthiasschaub added 9 commits July 6, 2021 11:14

Change type of attribute bpolys to Feature

1f80aa9

Add changes to breaking changes section

42af0ec

Simplify by removing unreached else branch

5a49c3c

Change variable name bpolys to feature

004d681

after load_bpolys() has been called.

Substitude Union[.., None] with Optional[Union[..

b2af057

Support FeatureCollection with one Feature

ac5d6f9

Minor code readability improvements

027db72

Remove TODO comment

c8c70a0

matthiasschaub force-pushed the bpoly-feature branch from 94d0737 to c8c70a0 Compare July 6, 2021 09:15

joker234 previously approved these changes Jul 6, 2021

View reviewed changes

matthiasschaub added 3 commits July 6, 2021 15:47

Change input tupe of parameter fid-field to string

8a64105

Update type hints

9450d8e

Add test for type_of function of db client

4252713

matthiasschaub dismissed joker234’s stale review via 4252713 July 6, 2021 13:47

matthiasschaub requested a review from joker234 July 6, 2021 13:47

joker234 approved these changes Jul 6, 2021

View reviewed changes

matthiasschaub merged commit 06b00ab into main Jul 6, 2021

matthiasschaub deleted the bpoly-feature branch July 6, 2021 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bpoly feature #69

Bpoly feature #69

matthiasschaub commented Jun 28, 2021 •

edited

joker234 Jul 5, 2021

matthiasschaub Jul 5, 2021

joker234 Jul 5, 2021

matthiasschaub Jul 6, 2021 •

edited

joker234 Jul 5, 2021

joker234 left a comment

matthiasschaub commented Jul 6, 2021

Bpoly feature #69

Bpoly feature #69

Conversation

matthiasschaub commented Jun 28, 2021 • edited

Description

Corresponding issue

Checklist

joker234 Jul 5, 2021

Choose a reason for hiding this comment

matthiasschaub Jul 5, 2021

Choose a reason for hiding this comment

joker234 Jul 5, 2021

Choose a reason for hiding this comment

matthiasschaub Jul 6, 2021 • edited

Choose a reason for hiding this comment

joker234 Jul 5, 2021

Choose a reason for hiding this comment

joker234 left a comment

Choose a reason for hiding this comment

matthiasschaub commented Jul 6, 2021

matthiasschaub commented Jun 28, 2021 •

edited

matthiasschaub Jul 6, 2021 •

edited