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

Issue421 improvements for specifying temporal extent with single string #468

Conversation

JohanKJSchreurs
Copy link
Contributor

@JohanKJSchreurs JohanKJSchreurs commented Sep 6, 2023

More improvements for issue #421

Follow up after PR #461

Overview of Changes

  • Made most functions internal, because they were indeed not actually intended to be user facing.
  • Documented the use of temporal extent with and without the shorthand notation for years and months in the page for data_access.html, and referring to in the getting started page.
  • Added test coverage via Connection.load_collection, DataCube.filter_temporal.
  • Usage like cube.filter_temporal("2020", "2022") or load_collection(..., temporal_extent=["2021-03", "2022-04"]) made sense so that is supported now as well.
  • In fact the simplest way to specify a year, for example temporal_extent="2023" wasn't actually supported, still had to use a tuple. So corrected that.
  • Update CHANGELOG so feature is mentioned.
  • Finally, moved the temporal extent functions to a separate module (split up openeo.util #465)

@JohanKJSchreurs JohanKJSchreurs marked this pull request as ready for review September 7, 2023 08:39
@JohanKJSchreurs JohanKJSchreurs marked this pull request as draft September 7, 2023 08:39
@JohanKJSchreurs
Copy link
Contributor Author

Closed accidentally, I hit the wrong button or something. So I reopened it.

@JohanKJSchreurs JohanKJSchreurs marked this pull request as ready for review September 7, 2023 14:29
@soxofaan
Copy link
Member

FYI: I pushed a commit on this feature branch to further finetune the docs (improve hierarchy a bit, moved some things around and tried to make some things more compact).

Doing it a commit seemed easier than trying to explain it as PR review :)

Allows taking better normalization decisions
@soxofaan
Copy link
Member

While reading the new docs and going through the tests, I'm starting to think that my suggestion from #421 (comment)

is usage like cube.filter_temporal("2020", "2022") or load_collection(..., temporal_extent=["2021-03", "2022-04"]) also handled: take first day of start date and last day + 1 for end date

was a bad idea. As you note in the docs and code comments, the behavior is now inconsistent and confusing I think:

  • normalize_extent("2019-03-15", "2019-10-11") would be handled left-closed -> [2019-03-15, 2019-10-11[
  • normalize_extent("2019-03", "2019-04") would be handled fully inclusive -> [2019-03-01, 2019-05-01[ instead of [2019-03-01, 2019-04-01[

I think we should work "left-closed" style everywhere.

As I'm knee-deep in the code already, I'm going to give it a go: starting with updating/extending the tests to make the outcomes more consistent, and then seeing if I can fix the implementation as well

…tency

The "round up" end_date feature as originally proposed
turned out to conflict too much with existing behavior and openEO spec.

Still, the idea can still be provided through single string `extent`: "2022" -> ("2022-01-01", "2023-01-01")
@JohanKJSchreurs
Copy link
Contributor Author

While reading the new docs and going through the tests, I'm starting to think that my suggestion from #421 (comment)

is usage like cube.filter_temporal("2020", "2022") or load_collection(..., temporal_extent=["2021-03", "2022-04"]) also handled: take first day of start date and last day + 1 for end date

was a bad idea. As you note in the docs and code comments, the behavior is now inconsistent and confusing I think:

* `normalize_extent("2019-03-15", "2019-10-11")` would be handled left-closed -> `[2019-03-15, 2019-10-11[`

* `normalize_extent("2019-03", "2019-04")` would be handled fully inclusive -> `[2019-03-01, 2019-05-01[` instead of `[2019-03-01, 2019-04-01[`

I think we should work "left-closed" style everywhere.

As I'm knee-deep in the code already, I'm going to give it a go: starting with updating/extending the tests to make the outcomes more consistent, and then seeing if I can fix the implementation as well

Much better indeed. With your improvements, I find the usage a lot easier to understand.
Looks good to me.

@soxofaan
Copy link
Member

ok thanks for the review,
I'll rebase and merge

soxofaan added a commit that referenced this pull request Sep 20, 2023
- restructure and change header hierarchy a bit
- trying to make paragraphs and snippets a bit more compact
@soxofaan
Copy link
Member

soxofaan commented Sep 20, 2023

rebased and merged in b734ea8

@soxofaan soxofaan closed this Sep 20, 2023
@soxofaan soxofaan deleted the issue421_improvements-for-specifying-temporal-extent-with-single-string branch September 20, 2023 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants