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.

Sign up for GitHub

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

Jump to bottom

Add option to select different datasets and fid fields as input to OQT #37

Merged
merged 41 commits into from
Jun 9, 2021
Merged

Add option to select different datasets and fid fields as input to OQT #37

merged 41 commits into from
Jun 9, 2021

Conversation

matthiasschaub
Copy link
Collaborator

@matthiasschaub matthiasschaub commented May 26, 2021 •
edited
Loading

Description

Add option to select different datasets and fid fields as input to OQT.

Available datasets and possible fid fields are defined in definitions.py.

Each dataset defaults to a primary fid field. The usage of other fields as identifier is possible.
The attribute/parameter/argument fid_field/fidField is added to the report class, oqt functions and API/CLI.

Rewrite save and load indicator from database logic to use one result table for all indicator results.

Corresponding issue

Closes #4

Checklist

  • 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
  • Check changes to VCR cassetts
  • Add sensible commits (squash, rebase, …)

@matthiasschaub matthiasschaub changed the title WIP Enable different datasets as input to OQT #4 May 26, 2021
@matthiasschaub matthiasschaub added the enhancement New feature or request label May 26, 2021
@matthiasschaub matthiasschaub self-assigned this May 26, 2021
@matthiasschaub matthiasschaub force-pushed the enable-different-datasets-as-input branch 4 times, most recently from 52ff5a0 to 575a0ef Compare June 1, 2021 15:26
@matthiasschaub matthiasschaub changed the title Enable different datasets as input to OQT #4 Add option to select different datasets and fid fields as input to OQT #4 Jun 1, 2021
@matthiasschaub matthiasschaub changed the title Add option to select different datasets and fid fields as input to OQT #4 Add option to select different datasets and fid fields as input to OQT Jun 1, 2021
@matthiasschaub matthiasschaub marked this pull request as ready for review June 1, 2021 15:40
@matthiasschaub matthiasschaub force-pushed the enable-different-datasets-as-input branch 3 times, most recently from 3ab57ed to 50c9fdd Compare June 3, 2021 13:56
joker234
joker234 reviewed Jun 4, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/base/report.py
@@ -40,9 +40,11 @@ def __init__(
bpolys: FeatureCollection = None,
dataset: str = None,
feature_id: int = None,
fid_field: str = None,
Copy link
Member

Choose a reason for hiding this comment

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

Where did you document what fid_field is supposed to be? Do we have documentation for these report parameters in general?

Copy link
Collaborator Author

@matthiasschaub matthiasschaub Jun 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Not really. The only documentation is provided through the CLI (cli_opts.py)

fid_field_opt = [
    click.option(
        "--fid-field",
        type=str,
        help=(
            "Provide the feature id field of the dataset. "
            + "Use command list-fid-fields to view available "
            + "fid fields for each dataset"
        ),
        default=None,
    )
]

workers/ohsome_quality_analyst/cli.py Show resolved Hide resolved
workers/ohsome_quality_analyst/cli.py Show resolved Hide resolved
workers/ohsome_quality_analyst/cli.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/cli_opts.py Show resolved Hide resolved
workers/ohsome_quality_analyst/geodatabase/create_results_table.sql Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/geodatabase/client.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/geodatabase/client.py Show resolved Hide resolved
matthiasschaub
matthiasschaub commented Jun 7, 2021
View reviewed changes
workers/ohsome_quality_analyst/api.py
async def get_bpolys_from_db(dataset: str, featureId: int):
bpolys = await db_client.get_bpolys_from_db(dataset=dataset, feature_id=featureId)
return bpolys
async def get_bpolys_from_db(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tests are not sufficient for this function. But I will remove the function in a separate PR since it is not in use.

@matthiasschaub matthiasschaub force-pushed the enable-different-datasets-as-input branch from 7b5bd3f to b5f0de8 Compare June 7, 2021 11:37
joker234
joker234 reviewed Jun 8, 2021
View reviewed changes
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 think you should consider redoing the VCRs with a fresh cassette again. I got some changed/additional requests when running pytest with a fresh database.

workers/ohsome_quality_analyst/geodatabase/client.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/geodatabase/client.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/geodatabase/client.py Show resolved Hide resolved
workers/ohsome_quality_analyst/geodatabase/create_results_table.sql Outdated Show resolved Hide resolved
@matthiasschaub matthiasschaub force-pushed the enable-different-datasets-as-input branch from fb30766 to ebe34ad Compare June 9, 2021 08:36
matthiasschaub and others added 13 commits June 9, 2021 10:39
@matthiasschaub @joker234
Improve docstring
de33296 
Update docstring

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>

Update docstring

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>
@matthiasschaub
Add test for custom CLI parameter type
19af023
@matthiasschaub
Init fid_field for tests in SetUp function
d34cd01
@matthiasschaub
Revert fid changes of tests
cfb54ef
@matthiasschaub
Add function to check sanity of user input
164596e
@matthiasschaub
Removed try except statement due to empty except
7a44cb2
@matthiasschaub
Revert variable name change
ccc89c1
@matthiasschaub
Update VCR cassettes
532be3b 
Recreate VCR cassetts
@matthiasschaub
Update CHANGELOG
d96cb12
@matthiasschaub
Rename attributes to avoid usage of SQL keywords
dbbbdd3
@matthiasschaub @joker234
Make if stetement more precise
30babd8 
Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>

Make if stetement more precise

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>
@matthiasschaub
Recreate relevant VCR cassettes
8ceb515
@matthiasschaub
Fix wrong test region for tests of oqt.py
08288ce
@matthiasschaub matthiasschaub force-pushed the enable-different-datasets-as-input branch from ebe34ad to 08288ce Compare June 9, 2021 08:39
@joker234 joker234 added this to the Release 0.4.0 milestone Jun 9, 2021
@matthiasschaub matthiasschaub merged commit bdc414a into main Jun 9, 2021
@matthiasschaub matthiasschaub deleted the enable-different-datasets-as-input branch June 9, 2021 09:37
@matthiasschaub matthiasschaub mentioned this pull request Jun 9, 2021
Closed
@matthiasschaub matthiasschaub mentioned this pull request Jul 6, 2021
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joker234 joker234 joker234 approved these changes

@Hagellach37 Hagellach37 Awaiting requested review from Hagellach37

Assignees

@matthiasschaub matthiasschaub

Labels
enhancement New feature or request
Projects
None yet
Milestone
Release 0.4.0
Development

Successfully merging this pull request may close these issues.

Enable different datasets as input to OQT
2 participants
@matthiasschaub @joker234