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

Indicator data input #245

Merged
merged 12 commits into from
Apr 12, 2022
Merged

Indicator data input #245

merged 12 commits into from
Apr 12, 2022

Conversation

matthiasschaub
Copy link
Collaborator

@matthiasschaub matthiasschaub commented Feb 3, 2022

Description

An Indicator consists of a Layer and a Feature.

This PR introduces a new Layer baseclass. Now a Layer object can be either an instance of a "Layer Definition" class (with information how to retrieve the data for the layer) or of a "Layer Data" class (with data already attached to itself). This way it is possible to compute an Indicator for given data. At the moment this is restricted to the MappingSaturation Indicator.

In particular the PR adds:

  • New Layer base-class and Layer sub-classes
  • New API parameter layer (containing name, description and data) to the endpoint indicator (POST)

Below is an example of the request body for a POST request to the /indicator endpoint.

{
  "name": "MappingSaturation",
  "bpolys": {
    "type": "Feature",
    "geometry": {
      "type": "Polygon",
      "coordinates": [
        [
          [
            8.674092292785645,
            49.40427147224242
          ],
          [
            8.695850372314453,
            49.40427147224242
          ],
          [
            8.695850372314453,
            49.415552187316095
          ],
          [
            8.674092292785645,
            49.415552187316095
          ],
          [
            8.674092292785645,
            49.40427147224242
          ]
        ]
      ]
    }
  },
  "layer": {
    "name": "",
    "description": "",
    "data": {
      "result": [
        {
          "value": 1,
          "timestamp": "2020-03-20T01:30:08.180856"
        },
        {
          "value": 56,
          "timestamp": "2020-03-20T01:30:08.180856"
        },
        {
          "value": 5581,
          "timestamp": "2020-03-20T01:30:08.180856"
        },
      ]
    }
  }
}

Corresponding issue

Closes #233

Checklist

@matthiasschaub matthiasschaub added enhancement New feature or request waiting An issue or PR which is waiting for an upstream bugfix, further information or is somehow blocked labels Feb 3, 2022
@matthiasschaub matthiasschaub self-assigned this Feb 3, 2022
@matthiasschaub matthiasschaub force-pushed the indicator-data-input branch 2 times, most recently from c8bdc53 to eca4957 Compare February 3, 2022 12:55
@matthiasschaub matthiasschaub force-pushed the indicator-data-input branch 2 times, most recently from 4b3e6fa to eca4957 Compare February 11, 2022 11:04
@matthiasschaub matthiasschaub force-pushed the indicator-data-input branch 2 times, most recently from 1d776bf to 05e75e5 Compare March 9, 2022 07:30
@matthiasschaub matthiasschaub force-pushed the indicator-data-input branch 7 times, most recently from 1e89d10 to c8e2f11 Compare March 14, 2022 16:29
@matthiasschaub matthiasschaub marked this pull request as ready for review March 14, 2022 16:29
@matthiasschaub matthiasschaub marked this pull request as draft March 16, 2022 06:41
@matthiasschaub matthiasschaub marked this pull request as ready for review March 16, 2022 15:18
@matthiasschaub matthiasschaub removed the waiting An issue or PR which is waiting for an upstream bugfix, further information or is somehow blocked label Mar 17, 2022
@GIScience GIScience deleted a comment from matthiasschaub Mar 22, 2022
Copy link
Member

@joker234 joker234 left a comment

Choose a reason for hiding this comment

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

First overview looks pretty good, but one question we have to clarify before it makes sense to review in more detail: Does it make sense to name the new parameter layer? I would assume it means the layer name and not layer data you can give the endpoint for calculation.

CHANGELOG.md Outdated Show resolved Hide resolved
@matthiasschaub matthiasschaub force-pushed the indicator-data-input branch 4 times, most recently from d393b69 to 711f7b0 Compare March 24, 2022 09:28
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@joker234 joker234 left a comment

Choose a reason for hiding this comment

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

I'm generally not really of using **_kargs or *_args, especially if you don't use them. Isn't there a better solution?

workers/ohsome_quality_analyst/api/request_models.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/api/request_models.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/oqt.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/oqt.py Show resolved Hide resolved
workers/tests/unittests/test_ohsome_client.py Outdated Show resolved Hide resolved
@matthiasschaub
Copy link
Collaborator Author

matthiasschaub commented Apr 8, 2022

I'm generally not really of using **_kargs or *_args, especially if you don't use them. Isn't there a better solution?

Those are used because of flags like the force flag which is an argument to some of the overloaded functions and not to others. I wanted to signal that this is the case.

E.g. Notice the different second arguments:

@create_indicator_as_geojson.register(IndicatorBpolys)
@create_indicator_as_geojson.register(IndicatorData)
async def _create_indicator_as_geojson(
    parameters: Union[IndicatorBpolys, IndicatorData],
    size_restriction: bool = False,
    **_kargs,
):
    pass


@create_indicator_as_geojson.register
async def _create_indicator_as_geojson(
    parameters: IndicatorDatabase,
    force: bool = False,
    **_kargs,
):
    pass

I am totally open for another solution.

PS: The caller of the function create_indicator_as_geojson always passes down the force flag. Even in the case where it is not needed.

Copy link
Member

@joker234 joker234 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Made only a few comments. If you want you can have a look at SonarCloud: https://sonarcloud.io/summary/new_code?id=ohsome-quality-analyst&pullRequest=245

workers/tests/unittests/test_ohsome_client.py Outdated Show resolved Hide resolved
workers/tests/unittests/test_ohsome_client.py Outdated Show resolved Hide resolved
workers/tests/unittests/test_ohsome_client.py Outdated Show resolved Hide resolved
workers/tests/unittests/test_ohsome_client.py Show resolved Hide resolved
joker234
joker234 previously approved these changes Apr 12, 2022
matthiasschaub and others added 11 commits April 12, 2022 14:59
attached to itself.

Factor out the layer classes from `base/indicator.py` to own module.
Instead of initializing an Indicator with a `layer_name` as string
initialize an Indicator with a `layer` as Layer object.
Also first instantiate a Layer object and then pass it as init parameter
to the Indicator object.
instead of making a request to the ohsome API to retrieve data

Call ohsome.query function with args not kwargs.
Make usage of singledispatch more concise.

Address review comments of the dispatch functions:
- Adhere to convention to name overloaded func '_'
- Remove # noqa comments
- Rename 'kargs' to 'kwargs'
- Add test cases for generic functions covering 'NotImplemented'
Ignore duplications in the tests fixtures.py

Fix timestamp range in test fixtures

Avoid loading the same fixture for each test case

In test do not use empty string for layer name
of the API instead of using empty strings.
@matthiasschaub matthiasschaub merged commit 3bd37b8 into main Apr 12, 2022
@matthiasschaub matthiasschaub deleted the indicator-data-input branch April 12, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new Endpoint: calculate indicator with given data
2 participants