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

Reworked and renamed LastEdit indicator to Currentness #178

Merged
merged 13 commits into from
Dec 2, 2021
Merged

Conversation

Gigaszi
Copy link
Contributor

@Gigaszi Gigaszi commented Sep 29, 2021

Description

Reworked and renamed the LastEdit indicator to Currentness. It now returns a histogram with the distribution of the latest contributions to an element for the whole timerange. New tresholds are defined.

Corresponding issue

Closes #

New or changed dependencies

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

Please check all finished tasks. If some tasks do not apply to your PR, please cross their text out (by using ~...~) and remove their checkboxes.

@Gigaszi Gigaszi mentioned this pull request Sep 29, 2021
5 tasks
@Gigaszi Gigaszi marked this pull request as ready for review October 5, 2021 09:53
@Gigaszi
Copy link
Contributor Author

Gigaszi commented Oct 6, 2021

In the current implementation here the indicator has a parameter, which allows to set a time range for the calculation. Currently the tests for test_api_indicator.py fail because of this implementation. To resolve this the parameter must also be set for all get/post/fetch functions of api.py. Do we want that or should the default value (last 10 years from current date) always apply?

@joker234 joker234 added this to the Release 0.7.0 milestone Oct 6, 2021
@joker234 joker234 added this to In progress in Indicator related issues via automation Oct 12, 2021
@joker234 joker234 self-requested a review October 18, 2021 08:23
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 your work 👍

Apart from the suggestions/questions I have some general remarks:

  • Before you do something else, have a look at the comments to the calculation and preprocessing of the indicator. Some suggestions might change other parts of your code as well.
  • All descriptions in the metadata.yaml have to be applied to the new indicator.
  • Some work should be put into the test. It's still called TestIndicatorLastEdit. You could think if it makes sense to check the actual results of your indicator. But this isn't a necessity, just worth thinking about.
  • Why did you implement the parameter only into the CLI and not the API?
  • Tests for the new parameter are missing.
  • Changelog entry for the new parameter is missing.
  • Please check your indicator with the API and especially the website (e.g. by temporarily adding it to a report).
  • The output description needs to be aligned.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
workers/ohsome_quality_analyst/base/indicator.py Outdated Show resolved Hide resolved
Indicator related issues automation moved this from In progress to Review in progress Oct 19, 2021
@joker234
Copy link
Member

In the current implementation here the indicator has a parameter, which allows to set a time range for the calculation. Currently the tests for test_api_indicator.py fail because of this implementation. To resolve this the parameter must also be set for all get/post/fetch functions of api.py. Do we want that or should the default value (last 10 years from current date) always apply?

Sorry, missed that question before reviewing. Is there some kind of reasoning behind the last 10 years? Why not use all of the available data? Or 1 year? We could use a default time range like //P1Y if we want. This would give you not calendar years, but 1 year intervals from the first data timestamp. Alternatively you can automatically determine the first beginning of the next year. E.g. by using this: start_date = datetime.date(first_timestamp.year+1, 1, 1)

@joker234 joker234 added the breaking This will break previous versions. Documentation and Changelog update compulsory label Oct 25, 2021
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.

Some additional remarks, additionally to the not yet solved ones from the last review.

@Gigaszi Gigaszi requested a review from joker234 November 9, 2021 09:27
Be aware that there it is very likely that many map features are outdated.
You should carefully check this before using the data as it indicates bad
data quality in respect to currentness.
yellow: |
Some map features could be outdated in this regions, since only a smaller
fraction has been updated in the past year.
More than half of the $elements edited elements were last edited between one and four years ago.
Copy link
Member

Choose a reason for hiding this comment

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

Please use numbers or words for numbers, not both. Example:

On the MapAction Lakes Count over 50% of the 3.0 edited elements were last edited in the last 7 years. More than half of the $elements edited elements were last edited between one and four years ago. This refers to medium data quality in respect to currentness.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally it wouldn't say were edited in the last 1 years but were edited in the last year.

@Gigaszi Gigaszi dismissed joker234’s stale review December 2, 2021 14:18

Matthias approved

Indicator related issues automation moved this from Review in progress to Reviewer approved Dec 2, 2021
@Gigaszi Gigaszi merged commit 0679e65 into main Dec 2, 2021
@Gigaszi Gigaszi deleted the lastedit2.0 branch December 2, 2021 14:19
Indicator related issues automation moved this from Reviewer approved to Done Dec 2, 2021
matthiasschaub pushed a commit that referenced this pull request Jan 5, 2022
* Reworked and renamed LastEdit indicator to Currentness

* fix tests

* remove time_range parameter; add new calculation

* fix CLI

* fix tests

* implement feedback

* fix plot for website

* change plot to match the mapping saturation plot

* add comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This will break previous versions. Documentation and Changelog update compulsory
Development

Successfully merging this pull request may close these issues.

None yet

3 participants