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

Generalize result values of all Indicators by introducing a result class value #369

Merged
merged 4 commits into from
Jul 27, 2022
Merged

Generalize result values of all Indicators by introducing a result class value #369

merged 4 commits into from
Jul 27, 2022

Conversation

matthiasschaub
Copy link
Collaborator

@matthiasschaub matthiasschaub commented Jul 5, 2022 •
edited

Description

Every Indicator determines a result class value for itself.
The result class value range is 1 up to 5 and maps to the result labels.

Corresponding issue

Closes #321

New or changed dependencies

Checklist

@matthiasschaub matthiasschaub force-pushed the indicator-result-class branch 3 times, most recently from a2ad698 to 9adaff3 Compare July 6, 2022 09:36
@matthiasschaub matthiasschaub marked this pull request as ready for review July 6, 2022 09:36
joker234
joker234 reviewed Jul 6, 2022
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.

In general pretty good. Please create an issue for each indicator to adjust to 5 classes instead of 3.

workers/ohsome_quality_analyst/base/indicator.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/base/indicator.py Show resolved Hide resolved
workers/ohsome_quality_analyst/base/indicator.py Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/base/indicator.py Show resolved Hide resolved
workers/ohsome_quality_analyst/geodatabase/client.py Outdated Show resolved Hide resolved
@matthiasschaub matthiasschaub force-pushed the indicator-result-class branch 2 times, most recently from 7eb4f59 to 6a912e1 Compare July 6, 2022 15:36
@matthiasschaub
Copy link
Collaborator Author

In general pretty good. Please create an issue for each indicator to adjust to 5 classes instead of 3.

I created an issue for this.
#371

joker234
joker234 previously approved these changes Jul 7, 2022
View reviewed changes
@matthiasschaub matthiasschaub added priority:high Should be addressed as soon as possible (next release) indicator waiting for review This pull request urgently needs a code review labels Jul 12, 2022
This was referenced Jul 19, 2022
Draft
Merged
joker234
joker234 previously approved these changes Jul 26, 2022
View reviewed changes
@matthiasschaub
Copy link
Collaborator Author

@joker234 thank you for the review. I updated the branch and resolved existing conlicts (changelog, test_base_indicator.py and test_base_report.py. Also I rewrote two small tests to use pytest and not unittest and removing the depedency on the database of those two tests (test_base_indicator.py and test_base_report.py).
Coul you please have another look?

@matthiasschaub
add result class to indicators
3d28074 
Generalize result values of all Indicators by introducing a result class
value.
Every Indicator determines a result class value for itself.
The result class value range is 1 up to 5 and maps to the result labels.
@matthiasschaub
test: rewrite using pytest instead of unittest
065b428
@matthiasschaub matthiasschaub merged commit b876a77 into main Jul 27, 2022
@matthiasschaub matthiasschaub deleted the indicator-result-class branch July 27, 2022 14:36
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

Assignees
No one assigned
Labels
indicator priority:high Should be addressed as soon as possible (next release) waiting for review This pull request urgently needs a code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Generalize result values of all Indicators by introducing a result class value
2 participants
@matthiasschaub @joker234